Re: [PATCH] v4: C++: more location wrapper nodes (PR c++/43064, PR c++/43486)

[Jason Merrill <jason@redhat.com>]

On 12/13/18 3:12 PM, David Malcolm wrote:
> On Wed, 2018-12-12 at 15:37 -0500, Jason Merrill wrote:
>> On 12/7/18 3:13 PM, David Malcolm wrote:
>>> On Tue, 2018-12-04 at 18:31 -0500, Jason Merrill wrote:
>>>> On 12/3/18 5:10 PM, Jeff Law wrote:
>>>>> On 11/19/18 9:51 AM, David Malcolm wrote:
>>>
>>> [...]
>>>>> @@ -1058,6 +1058,9 @@ grokbitfield (const cp_declarator
>>>>> *declarator,
>>>>>          return NULL_TREE;
>>>>>        }
>>>>>    
>>>>> +  if (width)
>>>>> +    STRIP_ANY_LOCATION_WRAPPER (width);
>>>>
>>>> Why is this needed?  We should already be reducing width to an
>>>> unwrapped
>>>> constant value somewhere along the line.
>>>
>>> "width" is coming from cp_parser_member_declaration, from:
>>>
>>> 	      /* Get the width of the bitfield.  */
>>> 	      width = cp_parser_constant_expression (parser, false,
>>> NULL,
>>> 						     cxx_dialect >=
>>> cxx11);
>>>
>>> and currently nothing is unwrapping the value.  We presumably need
>>> to
>>> unwrap (or fold?) it before it is stashed in
>>>     DECL_BIT_FIELD_REPRESENTATIVE (value).
>>>
>>> Without stripping (or folding) here, we e.g. lose a warning and get
>>> this:
>>>     FAIL: g++.dg/abi/empty22.C  -std=gnu++98  (test for warnings,
>>> line 15)
>>
>> Why does that happen?  check_bitfield_decl ought to handle the
>> location
>> wrapper fine.  That's where it gets folded.
> 
> The unstripped location wrapper defeats this check for zero in
> check_field_decls within cp/class.c:
> 
> 3555	      if (DECL_C_BIT_FIELD (x)
> 3556		  && integer_zerop (DECL_BIT_FIELD_REPRESENTATIVE (x)))
> 3556		/* We don't treat zero-width bitfields as making a class
> 3557		   non-empty.  */

Aha.  I wonder if integer_zerop should look through location wrappers? 
Or alternately, abort if it gets one?

On a tangent, perhaps we also want a macro like TREE_CODE that looks 
through wrappers.  TREE_CODE_WRAPPED?  _NO_WRAP?  Other name ideas?

> 3558		;
> 3559	      else
> 
> leading it to erroneously use the "else" clause, which thus erroneously
> clears CLASSTYPE_EMPTY_P, leading to the loss of:
> 
> g++.dg/abi/empty22.C:15:6: warning: empty class 'dummy' parameter passing
>    ABI changes in -fabi-version=12 (GCC 8) [-Wabi]
>     15 |   fun(d, f); // { dg-warning "empty" }
>        |   ~~~^~~~~~
> 
> check_bitfield_decl is called *after* that check, so the folding there
> doesn't help.
> 
>>>>> @@ -656,6 +656,9 @@ add_capture (tree lambda, tree id, tree
>>>>> orig_init, bool by_reference_p,
>>>>>          listmem = make_pack_expansion (member);
>>>>>          initializer = orig_init;
>>>>>        }
>>>>> +
>>>>> +  STRIP_ANY_LOCATION_WRAPPER (initializer);
>>>>
>>>> Why is this needed?  What cares about the tree codes of the
>>>> capture
>>>> initializer?
>>>
>>> This is used to populate LAMBDA_EXPR_CAPTURE_LIST.  Without
>>> stripping,
>>> we end up with wrapped VAR_DECLs, rather than the VAR_DECLs
>>> themselves,
>>
>> Sure, that sounds fine.
>>
>>> and this confuses things later on, for example leading to:
>>>
>>>    PASS -> FAIL : g++.dg/cpp0x/lambda/lambda-type.C  -std=c++14
>>> (test for excess errors)
>>>    PASS -> FAIL : g++.dg/cpp0x/lambda/lambda-type.C  -std=c++17
>>> (test for excess errors)
>>
>> Confuses how?
> 
> Without stripping, we get extra errors for decltype() on identifiers in
> the capture-list, e.g.:
> 
>    [i] {
>      same_type<decltype(i),int>();
>      same_type<decltype((i)),int const&>();
>    };
> 
> cpp0x/lambda/lambda-type.C: In lambda function:
> cpp0x/lambda/lambda-type.C:48:27: error: 'i' is not captured
>     48 |     same_type<decltype((i)),int const&>();
>        |                           ^
> cpp0x/lambda/lambda-type.C:48:39: error: template argument 1 is invalid
>     48 |     same_type<decltype((i)),int const&>();
>        |                                       ^
> 
> These occur because, in capture_decltype, when searching for the decl for
> "i" here:
> 
>    tree cap = value_member (decl, LAMBDA_EXPR_CAPTURE_LIST (lam));
> 
> the LAMBDA_EXPR_CAPTURE_LIST contains a wrapper around "i" rather than "i"
> itself, and so "i" isn't found by value_member; "cap" thus becomes NULL_TREE,
> and thus the error.

Ah.  Just above that I notice,

   /* FIXME do lookup instead of list walk? */

We could make that change, i.e.

tree cap = lookup_name_real (DECL_NAME (decl), /*type*/0, /*nonclass*/1,
                              /*block_p=*/true, /*ns*/0, LOOKUP_HIDDEN);
...
if (cap && is_capture_proxy (cap))

Jason