Re: [PATCH] c++: Fix usage of CONSTRUCTOR_PLACEHOLDER_BOUNDARY inside array initializers [90996]

[Jason Merrill <jason@redhat.com>]

On 4/6/20 6:22 PM, Patrick Palka wrote:
> On Mon, 6 Apr 2020, Jason Merrill wrote:
> 
>> On 4/6/20 3:07 PM, Patrick Palka wrote:
>>> This PR reports that since the introduction of the
>>> CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag, we are sometimes failing to resolve
>>> PLACEHOLDER_EXPRs inside array initializers that refer to some inner
>>> constructor.  In the testcase in the PR, we have as the initializer for "S
>>> c[];"
>>> the following
>>>
>>>     {{.a=(int &) &_ZGR1c_, .b={*(&<PLACEHOLDER_EXPR struct S>)->a}}}
>>>
>>> where CONSTRUCTOR_PLACEHOLDER_BOUNDARY is set on the second outermost
>>> constructor.  However, we pass the whole initializer to replace_placeholders
>>> in
>>> store_init_value, and so due to the flag being set on the second outermost
>>> ctor
>>> it avoids recursing into the innermost constructor and we fail to resolve
>>> the
>>> PLACEHOLDER_EXPR within.
>>>
>>> To fix this, we could perhaps either call replace_placeholders in more
>>> places,
>>> or we could change where we set CONSTRUCTOR_PLACEHOLDER_BOUNDARY.  This
>>> patch
>>> takes the latter approach -- when building up an array initializer, it
>>> bubbles
>>> any CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag set on the element initializers up
>>> to
>>> the array initializer.  Doing so shouldn't create any new PLACEHOLDER_EXPR
>>> resolution ambiguities because we don't deal with PLACEHOLDER_EXPRs of array
>>> type in the frontend, as far as I can tell.
>>
>> Interesting.  Yes, that sounds like it should work.
>>
>>> Does this look OK to comit after testing?
>>
>> Yes.
>>
>> Though I'm seeing "after testing" a lot; what testing are you doing before
>> sending patches?
> 
> Sorry for the sloppiness -- I should be writing "after a full
> bootstrap/regtest" instead of "after testing" because I do indeed do
> some testing before sending a patch.  In particular, I usually run and
> inspect the outputs of
> 
>      make check RUNTESTFLAGS="dg.exp=*.C"
>      make check RUNTESTFLAGS="old-deja.exp=*.C"
>      make check RUNTESTFLAGS="conformance.exp=*ranges*"
> 
> in a build tree that is configured with --disable-bootstrap, as a quick
> smoke test for the patch.  Is this a sufficient amount of testing before
> sending a patch for review, or would you prefer that I do a full
> bootstrap/regtest beforehand?

You don't need to do a full bootstrap and run non-C++ testsuites, but 
please run the full libstdc++ testsuite.

Is there a reason you aren't using 'make check-c++'?

> In any case, I'll make sure to clearly convey the amount of testing that
> was done and is remaining in future patch submissions.

Thanks.

Jason