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
