This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 08/12] Iterator varobj_items by their availability
- From: Keith Seitz <keiths at redhat dot com>
- To: Yao Qi <yao at codesourcery dot com>, gdb-patches at sourceware dot org
- Date: Thu, 24 Apr 2014 13:28:29 -0700
- Subject: Re: [PATCH 08/12] Iterator varobj_items by their availability
- Authentication-results: sourceware.org; auth=none
- References: <1392367471-13527-1-git-send-email-yao at codesourcery dot com> <1392367471-13527-9-git-send-email-yao at codesourcery dot com>
On 02/14/2014 12:44 AM, Yao Qi wrote:
V2:
- Fix changelog entry.
- Add comments.
- Use XNEW.
- Fix typo.
- Update copyright year for new added file.
gdb:
2014-02-14 Pedro Alves <pedro@codesourcery.com>
Yao Qi <yao@codesourcery.com>
* Makefile.in (SFILES): Add varobj-iter-avail.c.
(COMMON_OBS): Add varobj-iter-avail.o.
* varobj-iter-avail.c: New file.
* varobj-iter.h (avail_varobj_get_iterator): Declare.
* varobj.c (varobj_get_iterator): Add one more argument
'lang_ops'. All callers updated.
Return iterator for available children.
This looks good to me, but I have one little thing I'd like to ask...
+static int
+varobj_value_unavailable (struct value *val)
+{
[snip]
+/* Implementation of the `next' method for available-children-only
+ varobj iterators. */
+
+static varobj_item *
+avail_varobj_iter_next (struct varobj_iter *self)
+{
+ struct avail_varobj_iter *dis = (struct avail_varobj_iter *) self;
+ int num_children = dis->lang_ops->number_of_children (self->var);
+ int raw_index = self->next_raw_index;
+ varobj_item *item = NULL;
+
+ for (; raw_index < num_children; raw_index++)
+ {
+ struct value *child_value
+ = dis->lang_ops->value_of_child (self->var, raw_index);
+
+ /* The "fake" children will have NULL values. */
+ if (child_value == NULL || !varobj_value_unavailable (child_value))
+ {
Maybe it's just me (and maybe because I am especially slow today), but I
find the use of double-negatives slow me/my comprehension down. The
above test reads, "if child_value is NULL or child_value is not
unavailable..."
What do you think about inverting the name of this function and the test
like so:
if (child_value == NULL || varobj_value_available (child_value))
/* ... */
Then that reads, "if child_value is NULL or the value is available,"
which is immensely easier for me to comprehend. [But maybe I'm the only
one?]
Keith