Re: [RFA] c++/12266 (again)

[Keith Seitz <keiths at redhat dot com>]

On 08/02/2011 09:08 AM, Tom Tromey wrote:
> Maybe 'static const char * const'?

Sure. I've never seen this done before, so I just (blindly) followed suit.

> Keith>  +typedef char *namep;
>
> defs.h already has 'char_ptr' for this purpose.

Thanks. Changed. [I even went looking for that!]

> Keith>  +static void
> Keith>  +replace_typedefs (struct demangle_parse_info *info,
>
> Indent this line somehow.

Yes, sorry, I missed that one.

> Keith>  +	  while (TYPE_CODE (TYPE_TARGET_TYPE (last)) == TYPE_CODE_TYPEDEF)
> Keith>  +	    last = TYPE_TARGET_TYPE (last);
>
> It seems like you could construct a test case where the type is a stub
> type, so TYPE_TARGET_TYPE == NULL, causing a crash.

I've added a check for this.

> Keith>  +	      /* If there is only one typedef for this anonymous type,
> Keith>  +		 do not substitute it.  */
>
> I don't understand this comment.
> How does this code check if this anonymous type only had one typedef?

A little re-arranging of the code (pushing the while loop into the 
is_anon clause) would be less confusing, I think. While that whole block 
deals with this, that comment is probably best deleted in favor of 
explanations closer to the actual code dealing with it.

As for determining if there is only one typedef for a type, the idea is 
to iterate over all the typedefs for the type. If the original type 
(otype) is the same as the "last" typedef seen (i.e., 
TYPE_CODE(TYPE_TARGET_TYPE (last)) != TYPE_CODE_TYPEDEF), then there is 
only one typedef associated with this type (well, at least in this chain 
of typedefs). meth-typedefs.exp does exactly this (defines typedefs of 
typedefs).

If there is a better/easier way, I'm all eyes!

> Keith>  +      (void) inspect_type (info, ret_comp, free_list);
>
> Don't cast to void.

Changed.

> Keith>  +      /* Free any memory allocated during typedef replacement.  */
> Keith>  +      if (!VEC_empty (namep, free_list))
>
> You don't need the VEC_empty check here; vec.h will do the right thing
> if you try to iterate an empty (or NULL) VEC.

Changed.

> Keith>  +  cleanup = make_cleanup (null_cleanup, NULL);
> Keith>  +  if (current_language->la_language == language_cplus)
> Keith>  +    {
> Keith>  +      char *canon = cp_canonicalize_string_no_typedefs (copy);
> Keith>  +
> Keith>  +      if (canon != NULL)
> Keith>  +	{
> Keith>  +	  name = canon;
> Keith>  +	  make_cleanup (xfree, name);
> Keith>  +	}
>
> This change in linespec checks current_language, but the other does not.
> Why is that?

I originally had both checking the language, but while testing, 
uncovered some problems with that. I removed the check around the 
problematic one but did not/forgot to do so with this one. I have 
removed it and verified that it does not impact test results. [And 
besides, there is no reason for this not to work with C anyway!]

Updated patch attached. Thank you for looking at this.

Keith

Attachment: cp_canonicalize_no_typedefs-5.patch
Description: Text document