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

[Nick Roberts <nickrob@snap.net.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