Re: [C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[Jason Merrill <jason@redhat.com>]

On 12/6/18 12:23 PM, Paolo Carlini wrote:
> Hi,
> 
> On 06/12/18 16:11, Jason Merrill wrote:
>> 2- Unfortunately I have to fix another buglet I recently introduced, 
>> completely similar to c++/88222 fixed by Marek. Well, at least we will 
>> not print anymore an empty '' when the unqualified_id is null because 
>> the field is unnamed.
>>
>>> -        error_at (declarator->id_loc,
>>> -              "%qE is neither function nor member function; "
>>> -              "cannot be declared friend", unqualified_id);
>>> +        if (unqualified_id && declarator)
>>> +          error_at (declarator->id_loc,
>>> +                "%qE is neither function nor member function; "
>>> +                "cannot be declared friend", unqualified_id);
>>> +        else
>>> +          error ("unnamed field is neither function nor member "
>>> +             "function; cannot be declared friend");
>>
>> I wonder if we want to use the 'name' variable here.
> 
> Well, the name variable doesn't seem that useful here because for the 
> new testcase it has that famous catch all value "type name" .
> 
> I have been thinking that here and in other places we could imagine 
> keeping only the declarator check and dropping the "name" check. 
> Probably it would work. But in *many* existing places we actually check 
> *only* the name thus I'm nervous about attempting that now...
> 
>>
>>> 3- In the non-static case too, when from grokdeclarator we are 
>>> calling FIELD_DECL and passing the location as first argument, I 
>>> think we want to likewise pass declarator->id_loc when available.
>>
>>> -        decl = build_decl (input_location,
>>> +        decl = build_decl (declarator
>>> +                   ? declarator->id_loc
>>> +                   : input_location,
>>
>> I think we want to put this in a local variable, to share with the 
>> static case and probably other places in grokdeclarator.
> 
> In the below I'm sharing it only with the static case, straightforward. 
> Moving it one level up doesn't seem that useful because we only have 
> rather safe IMHO unconditional uses either of input_location or of 
> declarator->id_loc at the moment... Again, I'm pretty sure there is room 
> for further clean-ups in this area, but, for 9, I'd rather take care of 
> a bunch of additional small issues which I already have in my TODO list, 
> in grokbitfield, for example, as already mentioned. By the way, if isn't 
> already clear, I have been changing location bits only when I already 
> have a set of testcases, constructed from our testsuite via (lenghty ;) 
> instrumented runs.
> 
> New version of the patch attached.

OK.

Jason