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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Paolo Carlini-3
Hi,

I'm bundling together 3 more. In attachment order:

1- Since we decided to explicitly print the wrong type, I suppose we
want to consistently do that in templates too.

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.

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.

The added/extended testcase exercise all of the above. Tested x86_64-linux.

Thanks, Paolo.

////////////////


CL_grokbitfield2 (766 bytes) Download Attachment
patch_grokbitfield2 (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Jason Merrill
On 12/6/18 5:49 AM, Paolo Carlini wrote:
> Hi,
>
> I'm bundling together 3 more. In attachment order:
>
> 1- Since we decided to explicitly print the wrong type, I suppose we
> want to consistently do that in templates too.

OK.

> 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.

> 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.

Jason

Reply | Threaded
Open this post in threaded view
|

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

Paolo Carlini-3
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.

Paolo.


patch_grokbitfield2_2 (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Jason Merrill
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