Repository navigation
Lifetime extension through a member of a temporary in GNU and Clang modes [GH #90] - #389
Open
Momen-G-Ar wants to merge 2 commits into
Open
Momen-G-Ar wants to merge 2 commits into
Momen-G-Ar wants to merge 2 commits into
Conversation
edgcpp#90] Value initialization of a class without a user-provided default constructor zero-initializes the object before calling the constructor. The interpreter cleared the storage but did not record the subobjects as initialized, so reading a member that the constructor does not set was rejected in a constant expression: struct A { constexpr A() {} int x; }; struct B : A { int y; }; static_assert(B().x == 0); // Previously rejected. The constructor-call setup in interpret.c now uses init_subobject_to_zero for such classes. Classes with a user-provided default constructor are cleared as before, but not marked. This is needed by the next commit, which stops treating every member of a class prvalue as initialized; without it, "B().y" above would be rejected as well (Clang's dcl.init.general/p16-cxx20, GH69890). Updated expectations: gnu cpp2a/constexpr-init18 now passes; clang dcl.init.general/p16-cxx20, SemaCXX/constexpr-value-init and class.inhctor.init/p1 no longer report false errors.
Under Core issue 616, selecting a member of a class prvalue yields an
xvalue, and binding a reference to it extends the lifetime of the whole
temporary. GNU and Clang modes kept the C++11 rule (a prvalue), so
const int &r = A().b;
bound to a copy of the member and destroyed A() at the end of the
full-expression. GCC 9 and Clang 4 implement the issue, so cmd_line.c now
enables selection_from_prvalue_is_xvalue when emulating those versions or
later. This reapplies the change backed out for EDGcpfe/29032.
That change regressed Clang's SemaCXX/constant-expression-cxx2a because,
for "." and ".*" on a class prvalue, the interpreter marked the whole
prvalue as initialized (eok_dot_field and eok_pm_field in interpret.c).
Since C++20 a constexpr constructor can leave members uninitialized, so
"Y(0,0).z1.n" folded to an indeterminate value instead of being diagnosed.
The marking is removed; the previous commit fixed the one case where the
per-subobject state was incomplete. The same bug already affected strict
C++20 mode ("static_assert(W().n == 0);" was accepted).
is_glvalue_for_auto_object and is_address_of_auto_object (exprutil.c) now
look through such an xvalue to the class prvalue, so "return A().b;" still
warns about returning a reference to a local temporary. The
traverse_addressing_subtree comments (il_walk.c) are updated to match.
Tests: regressions/edgcppgh-90 (lifetime, including the GCC 9 and Clang 4
modes), regressions/edgcppgh-90-old-modes ("A().b" is still a prvalue for GCC
8.3 and Clang 3.9), regressions/edgcppgh-90-warn (warnings), cwg/cwg616. Updated
expectations: gnu init/ref19 now passes; clang drs/cwg6xx no longer
rejects the Core issue 616 cases; clang expr.const/p2-0x now diagnoses a
read of a volatile prvalue's member; clang cxx2c-placeholder-vars loses an
"expression has no effect" warning on "S{}._", as in strict mode; a note
caret moves to the member in SemaCXX/constant-expression-cxx2a; gnu
cpp0x/constexpr-delegating3 (delegation to a defaulted constructor, a
separate bug) still fails, but with "access to uninitialized object"
instead of a garbage value.
Collaborator
|
AcknowlEDG review results updated for
CI other. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Member selection rules and CWG 616 tests src/cmd_line.c, src/exprutil.c, src/il_walk.c, tests/tests/cwg/*, tests/tests/imported/clang/cpp/CXX/drs/.cwg6xx.rto/* |
C++14 mode enables xvalue selection for the specified GNU and Clang versions. Traversal follows the configured addressing path. CWG 616 tests check direct and pointer-to-member access, and expected diagnostics are updated. |
Initialization and constant-evaluation behavior src/interpret.c, src/pending_changes/gh-90-*.txt, tests/tests/imported/clang/cpp/CXX/dcl.decl/..., tests/tests/imported/clang/cpp/CXX/expr/expr.const/*, tests/tests/imported/clang/cpp/SemaCXX/*, tests/tests/imported/gnu/cpp/* |
Value initialization now recursively zero-initializes classes without user-provided default constructors. Addressing members of class rvalues no longer marks the complete object initialized. Related expected diagnostics and issue notes are updated. |
Lifetime and warning regressions tests/tests/regressions/gh-90*.sft.cpp, tests/tests/regressions/.gh-90*.rto/* |
Regression tests check temporary lifetime for references to members and array elements across compiler versions. They also check warnings for returning references or pointers into temporary members. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- edgcpp/compiler#388: Both changes cover analysis of references to temporary objects, including references to temporary members.
Suggested reviewers: darkarc
Merge Risk: 
4bea6
-
The change appears mergeable after normal checks. The supplied evidence does not establish a remaining behavior regression in the targeted compiler modes.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 files. (59 skipped:… 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: lifetime extension through members of temporary objects in GNU and Clang modes, with the related issue identified. Linked Issues check Issue #90requires that binding a reference to a member of a class temporary (const int &r = A().b;) extends the lifetime of the class temporary, as GCC and Clang do. The only directly linked issue …Out of Scope Changes check The changes stay within the scope of GH #90.
-src/cmd_line.c,src/exprutil.c,src/il_walk.c(comments only): these changes implement Core issue 616 and the traversal needed to keep the return-…Full details: Linked Issues check
-
Explanation
Issue
#90requires that binding a reference to a member of a class temporary (const int &r = A().b;) extends the lifetime of the class temporary, as GCC and Clang do. The only directly linked issue is#90, and it is open. Its requirement is a coding requirement.The PR meets this requirement for the GNU and Clang emulation modes:
src/cmd_line.c:selection_from_prvalue_is_xvalueis enabled when rvalue references are enabled and the emulated compiler is GCC 9.0 or later, or Clang 4.0 or later. This applies Core issue 616, soA().bis an xvalue and the reference extends the temporary's lifetime.src/exprutil.c:is_glvalue_for_auto_objectandis_address_of_auto_objectfollow the class prvalue through the new xvalue. This keeps the warning for returning a reference to a local temporary.src/interpret.c: removes the "fully initialized" marking for.and.*on a class prvalue, and zeroes value-initialized classes. This keeps the constant evaluator correct after the change.- Regression tests
regressions/gh-90.sft.cppandregressions/gh-90-warn.sft.cppcover the lifetime case for g++ 9+ and Clang 4+.regressions/gh-90-old-modes.sft.cppchecks that older emulated versions and strict mode keep their behavior.
The PR description reports that the full suite passes on commit 4bea665 (GitHub Actions run 40175117293). This report is author-supplied. I did not verify the CI result. The code changes in the summary match the requirement.
No unmet requirement was found. The issue's example does not state an emulation mode. The PR description states that strict mode already extends the lifetime, so the reported failure must occur in GNU or Clang mode, which this PR addresses.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within the scope of GH
#90.src/cmd_line.c,src/exprutil.c,src/il_walk.c(comments only): these changes implement Core issue 616 and the traversal needed to keep the return-to-temporary warnings. They are in scope.src/interpret.cvalue-initialization change: the PR description states that this is a prerequisite for removing the shortcut ineok_dot_fieldandeok_pm_field. Without it, removing the shortcut rejectsB().yindcl.init.general/p16-cxx20(GH69890). This is a justified supporting change.src/pending_changes/gh-90-*.txt: documentation entries for the two commits. They are in scope.- Updated expectation files under
tests/tests/imported/...andtests/tests/regressions/...: these record diagnostic changes that follow directly from the lifetime and constant-evaluation changes. The PR description lists each one with its cause. - Deletion of
edg_x86_32.1.1.txtandedg_x86_32_cp.1.1.txtforgnu/cpp/cpp0x/constexpr-delegating3: the PR states that these recordings are now identical todefault.1.1.txt. This is a consequence of the change. - Related but separate follow-ups (Core issue 1213, member function calls on class prvalues, delegation to a defaulted constructor) are excluded from this PR. The diff does not include them.
I did not inspect the full diff. This verdict relies on the change summaries and the PR description. The file
tests/expectations/edg_x86_64/imported.logis in ignored_files and was not inspected. It does not affect this verdict.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 files. (59 skipped: 56 unsupported, 3 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.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes
GH #90([GNATS 29059]): in GNU and Clang modes, binding a reference to a member of a class temporary did not extend the temporary's lifetime ([class.temporary]/6).--g++(GCC 9+),--clang(Clang 4+)rbound to a copy ofb,A()destroyed at end of full-expression--g++(GCC < 9),--clang(Clang < 4)Root cause
A().bwas still a prvalue (the C++11 rule), so the reference bound to a copy of the member. GCC 9 and Clang 4 implement Core issue 616 (A().bis an xvalue). daveed's fix for EDGcpfe/29032 enabled the rule for those versions incmd_line.cbut was backed out because it regressed Clang'sSemaCXX/constant-expression-cxx2a.--clang --clang_version 999999, so with thecmd_line.cchangeY(0,0).z1.ntakes the xvalue path in the constexpr interpreter. For.and.*on a class prvalue, that path calledmark_complete_object_initializedon the prvalue. The shortcut dates from before C++20 (P1331), when a constexpr constructor had to initialize every member; now it need not, so the read of the uninitializedz1.nfolded to a garbage value (the final comparison was -48039863565308959547770532813896688677 == 0) instead of being diagnosed. The same bug was already present in strict--c++20mode (static_assert(W().n == 0);was accepted forstruct W { int n; constexpr W() {} };).B().yin Clang'sdcl.init.general/p16-cxx20(GH69890).Commits
interpret.c): prerequisite for commit 2. Value initialization of a class without a user-provided default constructor now usesinit_subobject_to_zero. Classes with a user-provided default constructor are cleared as before but not marked.cmd_line.c,interpret.c,exprutil.c,il_walk.c): the fix for the issue.cmd_line.c: daveed's change.interpret.c: remove the "fully initialized" marking for.and.*on a class prvalue (eok_dot_field,eok_pm_field), which is what made the change regress.exprutil.c:is_glvalue_for_auto_objectandis_address_of_auto_objectlook through the new xvalue to the class prvalue, soreturn A().b;from a function returningconst int &still warns about returning a reference to a local temporary.il_walk.c: comments only.Each commit has its own Changes entry in
src/pending_changes/(gh-90-value-init-zeroed-members.txt,gh-90-lifetime-extension-member-of-temporary.txt; both passedg-collect-changes --check), builds, and passes its tests on its own. Based onmainat 35b7c47.Changes to existing expectations
All are improvements, except that
constexpr-delegating3still fails, now with a different diagnostic.cpp2a/constexpr-init18(fp)dcl.init.general/p16-cxx20(fn)B().xremoved (GH69890)SemaCXX/constexpr-value-init(fn)B()andE()(// ok) removedclass.init/class.inhctor.init/p1(fn)constexpr Wrap2 b = {};removedinit/ref19(rp, PR 49528)drs/cwg6xx(fn)expr.const/p2-0x(fn)SemaCXX/cxx2c-placeholder-vars(fn)S{}._no longer issued in GNU mode, as in strict modeSemaCXX/constant-expression-cxx2a(fn).z1to.ncpp0x/constexpr-delegating3(fp)default.1.1.txtVersion gating
Checked on Compiler Explorer with
-std=c++14:A().bis a prvalue in GCC 8.3 and Clang 3.9, and an xvalue in GCC 9.2 and Clang 4.0 and later.Tests
regressions/gh-90.sft.cppA().b,A().i[0],A().*pm,A().i: C++14/C++20, strict, g++ 9/15, Clang 4/19regressions/gh-90-old-modes.sft.cppA().bis still a prvalue when emulating GCC 8.3 and Clang 3.9, just below the version thresholds (fails for GCC 9.0 and Clang 4.0)regressions/gh-90-warn.sft.cppcwg/cwg616.sft.cppmainand pass with this change, exceptgh-90-old-modes, which checks behavior that does not change (edg-docker-test,edg_x86_64andedg_x86_64_cp).edg-docker-test -W, and thedefault.*files were reviewed.Build & Test CI, all four configurations): passes on 4bea665 (run 40175117293), rebased onmain35b7c47. Strict Build Check, Python Typecheck and Pending Changes Entries also pass.Not in this PR
Found while working on this; to be raised separately:
A().i[0]is an lvalue instead of an xvalue, soconst int &r = c ? A().b : A().i[0];binds to a copy in all modes.eval_selector_arg): the same "fully initialized" marking remains, sostatic_assert(W().get() == 0);is still accepted in--c++20.constexpr-delegating3).Closes: #90
Summary by CodeRabbit