This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 08/12] Iterator varobj_items by their availability


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]