Re: RFC: MI - Detecting change of string contents with variable objects

[Nick Roberts <nickrob at snap dot net dot nz>]

 > > + ? /* Last print value */
 > 
 > Dot and two spaces ;-)

OK 

 >...
 > > + static char *value_get_print_value (struct value *value,
 > > + ?????????????????????????????? ? ?enum varobj_display_formats format);
 > > + 
 > 
 > I don't see a comment to this function either here or at the definition
 > point.

Too many comments get in the way of the code and I think the name is fairly
self explanatory.  Most of the simple functions don't have a comment and AFAIK
a comment for them is not a GNU or GDB coding standard.

 > > ? static int type_changeable (struct varobj *var);
 > > ? 
 > > ? /* C implementation */
 > > *************** install_new_value (struct varobj *var, s
 > > *** 978,1003 ****
 > > ? ?????? ? ?changed = 1;
 > > ? ?????? ?else
 > > ? ?????? ? ?{
 > > ? ?????? ? ? ?gdb_assert (!value_lazy (var->value));
 > > - ?????? ? ? ?gdb_assert (!value_lazy (value));
 > 
 > Did you remove this because it failed? If so, I'd like to understand why.
 > I think this assert is needed -- if the value is lazy, then even if
 > printing code will fetch the value, you'll be comparing current value with
 > current value. That's a definite bug, so must be asserted.

I removed it accidentally.  I've put it back.

 > > ? ?????? ? ? ?
 > > ! ?????? ? ? ?if (!value_contents_equal (var->value, value))
 > > ! ??????????????changed = 1;
 > > ? ?????? ? ?}
 > > ? ??????}
 > > ? ? ? }
 > > ! ? ? 
 > > ? ? /* We must always keep the new value, since children depend on it. ?*/
 > > ? ? if (var->value != NULL)
 > > ? ? ? value_free (var->value);
 > > ? ? var->value = value;
 > > ? ? var->updated = 0;
 > > ! ? 
 > > ? ? gdb_assert (!var->value || value_type (var->value));
 > 
 > Is that a formatting change above?

I've just removed unnecessay spaces.  The real change is replacing
value_contents_equal (but what's with all the underscores!).

 > > ! ?????????????? ?if (strcmp (var->print_value, print_value))
 > 
 > Can you use 
 > 
 >         strcmp (var->print_value, print_value) != 0

Is that more legible?  I sometimes see "if (fi != NULL)" but "if (fi)"
seems clearer.  Maybe it comes from programming in Lisp for Emacs.

 > for legibility?
 > 
 > > ! ?????????????? ? ?{
 > > ! ?????????????? ? ? ?xfree (var->print_value);
 > > ! ?????????????? ? ? ?var->print_value = print_value;
 > > ! ?????????????? ? ? ?changed = 1;
 > > ! ?????????????? ? ?}
 > 
 > So, if values differ you xfree the old one and assign the new one. If the
 > values are the same -- where is 'print_value' freed?

It's not; it's a legacy of earlier code.  I'll change it.

-- 
Nick                                           http://www.inet.net.nz/~nickrob