Repository navigation
Bind an NSDMI's implicit this to the right aggregate [GH #324] - #408
Momen-G-Ar wants to merge 1 commit into
Conversation
When lowering a non-constant aggregate initialization, lower_param_ref
replaced the enk_param_ref for "this" with the innermost aggregate on
aggregate_this_stack whose type matched. A default member initializer
that aggregate-initializes another object of the same class then bound
the implicit "this" of its explicit elements to that nested object:
struct Y { unsigned k; unsigned n = Y{2, k}.n; };
Y *p = new Y{1}; // p->n was 2, not 1.
Only an element that comes from a default member initializer (marked
implicit_aggr_element) can refer to the aggregate it initializes, which
is also the rule the constexpr interpreter uses. An aggregate is now
skipped in the search while one of its explicit elements is lowered;
if nothing else matches, the previous search is used.
|
AcknowlEDG review results updated for
CI other. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Track eligible aggregate candidates src/lower_il.h, src/lower_init.c |
Initialization-position state tracks when an explicitly initialized class aggregate is excluded as an implicit-this candidate. The state is cleared when leaving the aggregate. Anonymous unions remain eligible. |
Resolve references and add regression coverage src/lower_il.c, tests/tests/regressions/gh-324.sft.cpp, tests/tests/regressions/.gh-324.rto/*, src/pending_changes/gh-324-nsdmi-this-nested-aggregate.txt |
lower_param_ref searches eligible aggregate entries before retrying against the full stack. Regression coverage checks several aggregate forms, and the pending-changes note records the reported result change. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Suggested labels: bug
Suggested reviewers: daveedvdv-nvidia
Merge Risk: 
e7c78
-
No merge-blocking issue was established for the aggregate lookup change.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: binding an NSDMI's implicit thisto the correct aggregate for GH#324.Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 2 files. (9 skipped: 7 unsupported, 2 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
AcknowlEDG review results updated for
CI success. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Summary
Fixes the bug reported in discussion #324 (no issue exists yet; the commit and Changes entry are tagged
[GH #324]after the discussion, and I'll retag them once an issue is opened): when the default member initializer of an aggregate class aggregate-initializes another object of the same class, the implicitthisin the elements of that nested initializer referred to the nested object instead of to the object being initialized.khere isthis->k, andthisis the aggregate whose element is initialized from the default member initializer ([dcl.init.aggr]/5.1). g++ and Clang given == 1.Y y{1};,Y a = Y{1};(folded to a constant by the front end)new Y{1},return Y{1};, inner object passed by value or byconst Y &(lowered)thisbound to the nestedY{2, k}Y y{v};, a global,return {v};, a mem-initializer, a member of another aggregate, a class templatethisbound to the nestedY{2, k}Same result in strict,
--g++and--clangmodes.Root cause
lower_param_ref(lower_il.c) replaces theenk_param_refforthisin a non-constant aggregate initialization with the innermost entry onaggregate_this_stackwhose type matches. A comment there assumed that "there can only be one". In the example, the innerY{2, k}pushes a secondYentry (inlower_dynamic_init_aggregate_constant,lower_init.c) on top of the outer one before its explicit elementkis lowered, sothisbound to the inner temporary.Change
Only an element that comes from a default member initializer, which the front end marks with
implicit_aggr_element, can refer to the aggregate it initializes. An element written in the braces refers to thethisof the enclosing context. The constexpr interpreter already uses this rule (interpret.c,set_up_param_ref_for_this_ptris called only forimplicit_aggr_elementelements).lower_il.h: newan_init_pos_descr::not_this_candidateflag, cleared inclear_init_pos_descr.lower_init.c:lower_dynamic_init_aggregate_constantsets the flag on its stack entry while an explicit element is lowered. Anonymous unions are not affected, since their members' default member initializers refer to the enclosing class.lower_il.c: the stack search moves toreplace_param_ref_with_aggregate_this. It skips flagged entries first; if nothing matches, it repeats the previous search, so any case not covered gives the same result as before.Alternatives considered
Wcase in the test).enk_param_refwhich aggregate it belongs to, in the front end. Larger and more intrusive, for a problem that exists only in the lowering; the front end and the constexpr interpreter already have the information they need (implicit_aggr_element).The fallback to the previous search keeps any unforeseen path (an NSDMI-derived element without
implicit_aggr_element) behaving exactly as before, instead of reachingctor_init_thisor an internal error.Changes entry:
src/pending_changes/gh-324-nsdmi-this-nested-aggregate.txt(passesedg-collect-changes --check). Based onmainat 35b7c47; merges cleanly with the currentmain. The commit message passesedg-check-commit-messages.Tests
regressions/gh-324.sft.cppnew, return, by-value and by-reference cases; foldedY y{1}; run-time values (local, global,return {v}, by-value argument, mem-initializer, member of another aggregate, class template); the inner object's own default member initializer still binds to the inner object (W); anonymous union, aggregate base class, lambda capturingthis, arraynew.--c++20strict,--g++ 15,--clangimported/gnu/cpp/cpp1y/pr79937-4.sft.cpp--c++14. It was expected to fail and now passes; itsFAILline is removed fromtests/expectations/edg_x86_64/imported.log.The
Wcheck is skipped in--clangmode: there, a member skipped by a designated initializer does not get its default member initializer (W{.k = 2, .n = 0}.jis 0). That happens without this change too and is the issue in discussion #259.mainand passes with this change (edg-docker-test,edg_x86_64andedg_x86_64_cp, all three variants).edg-docker-test -W, and thedefault.*files were reviewed.--g++,--clang; C and C++ generating back ends) gives the same output as g++ and Clang with this change, and differs in every mode without it.this) match their expectations inedg_x86_64andedg_x86_64_cp.Build & Test CI, all four configurations): passes on e7c7820 (run 40556389927). Strict Build Check, Python Typecheck and Pending Changes Entries also pass.Discussion: #324
Summary by CodeRabbit
thisreferences are resolved in default member initializers when constructing nested aggregates of the same class. References now point to the object being initialized rather than an unintended nested object, including in runtime-valued initialization and return expressions.