Re: [PATCHv2 4/5] gdb: Introduce new language field la_is_string_type_p

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Pedro Alves <palves@redhat.com> [2019-04-18 18:07:55 +0100]:

> On 4/17/19 12:06 AM, Andrew Burgess wrote:
> > This commit is preparation work for the next commit, and by itself
> > makes no user visible change to GDB.  I've split this work into a
> > separate commit in order to make code review easier.
> > 
> > This commit adds a new field 'la_is_string_type_p' to the language
> > struct, this predicate will return true if a type is a string type for
> > the given language.
> > 
> > Some languages already have a "is this a string" predicate that I was
> > able to reuse, while for other languages I've had to add a new
> > predicate.  In this case I took inspiration from the value printing
> > code for that language - what different conditions would result in
> > printing something as a string.
> > 
> > A default "is this a string" method has also been added that looks for
> > TYPE_CODE_STRING, this is the fallback I've used for a couple of
> > languages.
> > 
> > In this commit I add the new field and initialise it for each
> > language, however at this stage the new field is never used.
> > 
> 
> Thanks.  This nicely addresses one of my previous review comments.
> 
> I can't speak to whether the implementation for the different languages
> are correct.  E.g., I'm curious from where you extracted the M2 and
> the Rust bits.  Didn't seem like it was a refactoring job?

It was mostly me looking for calls to LA_PRINT_STRING in each
languages value printing code then combining all of the conditions
required to reach these calls.

Could we refactor to make use of these new is_string functions within
the value printing code?  Sure, but I don't think it would be pretty.
Most languages base their value printing around a switch statement,
and also have additional format related conditions mixed in with the
logic.

Ideally, at least for this commit I'd prefer to no mess with the value
printing code more than I need too...

Thanks,
Andrew




> 
> Formatting comments below.
> 
> > +/* Return true if TYPE is a string.  */
> > +static bool
> 
> Empty line after comment.
> 
> > +static bool
> > +m2_is_string_type_p (struct type *type)
> > +{
> > +  type = check_typedef (type);
> > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> > +      && TYPE_LENGTH (type) > 0
> > +      && TYPE_LENGTH (TYPE_TARGET_TYPE (type)) > 0)
> > +    {
> > +      struct type *elttype = check_typedef (TYPE_TARGET_TYPE (type));
> > +
> > +      if (TYPE_LENGTH (elttype) == 1 &&
> 
> && goes on the next line.
> 
> > +	  (TYPE_CODE (elttype) == TYPE_CODE_INT
> > +	   || TYPE_CODE (elttype) == TYPE_CODE_CHAR))
> > +	return true;
> > +    }
> > +
> > +  return false;
> > +}
> > +
> 
> Thanks,
> Pedro Alves