wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.
very good start!
================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:5-6
+FORWARD_LIST = "FORWARD_LIST"
+LIST = "LIST"
+
----------------
remove this
================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:21-151
+ def next_node_abstract(self, node):
logger = lldb.formatters.Logger.Logger()
return node.GetChildMemberWithName('_M_next')
- def is_valid(self, node):
+ def is_valid_abstract(self, node, break_condition):
logger = lldb.formatters.Logger.Logger()
+ valid = self.value_abstract(self.next_node_abstract(node)) !=
break_condition
----------------
remove all these _abstract suffixes. They make reading harder
================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:27
logger = lldb.formatters.Logger.Logger()
- valid = self.value(self.next_node(node)) != self.node_address
+ valid = self.value_abstract(self.next_node_abstract(node)) !=
break_condition
if valid:
----------------
here you should call the method get_end_of_list_address that i mention below
================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:41-42
- # Floyd's cycle-finding algorithm
- # try to detect if this list has a loop
- def has_loop(self):
----------------
don't remove this command. It's useful to know what's going on
================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:95
current = current.GetChildMemberWithName('_M_next')
- return (size - 1)
+ return (size - size_correction)
except:
----------------
what is this? try to come up with a better name with documentation to make this
easier to understand
================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:161
+ def num_children(self):
+ return super().num_children_abstract(incoming_size=1,
size_correction=0, is_valid_break_condition=0, type=FORWARD_LIST)
+
----------------
don't pass the is_valid_break_condition variable everywhere. It makes code more
confusing. Instead, you should create a method "get_end_of_list_address" that
should be overridden by each child implementation. Then you add a comment that
this address is used to identify if a node traversal has reached its end
================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:161
+ def num_children(self):
+ return super().num_children_abstract(incoming_size=1,
size_correction=0, is_valid_break_condition=0, type=FORWARD_LIST)
+
----------------
wallace wrote:
> don't pass the is_valid_break_condition variable everywhere. It makes code
> more confusing. Instead, you should create a method "get_end_of_list_address"
> that should be overridden by each child implementation. Then you add a
> comment that this address is used to identify if a node traversal has reached
> its end
don't pass 'type' here, instead, in the constructor of the parent in line 158
pass a boolean flag 'has_prev'. That should be enough for the parent class to
know if there's a prev pointer or not
================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:161
+ def num_children(self):
+ return super().num_children_abstract(incoming_size=1,
size_correction=0, is_valid_break_condition=0, type=FORWARD_LIST)
+
----------------
wallace wrote:
> wallace wrote:
> > don't pass the is_valid_break_condition variable everywhere. It makes code
> > more confusing. Instead, you should create a method
> > "get_end_of_list_address" that should be overridden by each child
> > implementation. Then you add a comment that this address is used to
> > identify if a node traversal has reached its end
> don't pass 'type' here, instead, in the constructor of the parent in line 158
> pass a boolean flag 'has_prev'. That should be enough for the parent class to
> know if there's a prev pointer or not
similarly, pass incoming_size to the constructor, and a better name is
"node_value_pointer_offset", and you add a comment explaining that lists have 1
or more pointers followed by the value
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113362/new/
https://reviews.llvm.org/D113362
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
