fix: recognize last uses (including of `this`) #887

JohelEGP · 2023-12-12T06:28:08Z

Resolves #890.
Resolves #888.
Resolves #884.
Resolves #869.
Resolves #857.
Resolves #850.
Resolves #847.
Resolves #832.
Resolves #825.
Resolves #313.
Resolves 13f765b#commitcomment-133825753.
Resolves 99d66f0#diff-b45bd40d0ae8fbf75e5dcf83e067c0f9a715d4f46e05ffe839d487878f0dcc44R31-R35.

Testing summary:

100% tests passed, 0 tests failed out of 898

Total Test time (real) =  69.23 sec

Acknowledgements:

Tested thanks to https://github.com/modern-cmake/cppfront.
Commit messages follow Conventional Commits 1.0.0.
Comments and commit messages follow Semantic Line Breaks.

regression-tests/test-results/pure2-deducing-pointers-error.cpp2.output

hsutter

Thanks! I've taken a first pass just at:

getting a clean compile
answering a couple of your questions
asking a couple of new questions

I thought I'd push this review (and some fixes I pushed now) for you to take a look, then I can dive into the logic itself. Thanks!

hsutter · 2024-01-01T23:23:49Z

regression-tests/test-results/gcc-13/pure2-last-use.cpp.output

+pure2-last-use.cpp2: In member function ‘void issue_857::o2() &&’:
+pure2-last-use.cpp2:128:47: error: ‘std::move’ is not a class member
+  128 |   o2:(move this) = 0.n();
+      |                                               ^  


Are these still being investigated?

From #746 (comment), I opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111841,
which depends on Bug 98939 (P1787R6) - [C++23] Implement P1787R6 "Declarations and where to find them" , opened in 2021-02-02.
Progress seems slow.

source/sema.h

hsutter · 2024-01-01T23:54:53Z

source/sema.h

-        if (t.type() != lexeme::Identifier) {
+        if (t.type() == lexeme::Dot) {
+            started_member_access = true;
+            started_this_member_access = *(&t - 1) == "this";


Past parsing, I try not to abuse x-ray vision to the previous token too often... it's pointer arithmetic after all, which wouldn't be allowed in Cpp2 code so would need to be fixed when the code is a self-hosting candidate. But yes, I do this very sparingly right now when it makes sense and there's no convenient alternative.

hsutter · 2024-01-01T23:58:50Z

source/sema.h

-        if (t.type() != lexeme::Identifier) {
+        if (t.type() == lexeme::Dot) {
+            started_member_access = true;
+            started_this_member_access = *(&t - 1) == "this";


In this case, since it's handling individual tokens anyway, perhaps just to remember the last_token right in this same function as another visitor state? I realize visitors aren't always ideal but it seems to be sufficient for this for now and that would likely be the smallest diff that avoids the pointer arithmetic.

source/to_cpp1.h

hsutter · 2024-01-02T00:04:39Z

regression-tests/pure2-last-use.cpp2

+  n: (move this) = { }
+  o0:(move this) = n();
+  o1:(move this) = this.n();
+  o2:(move this) = 0.n();


For this line, when I tried compiling the test using cppfront+MSVC, cppfront was fine but MSVC complained:

pure2-last-use.cpp2(193): error C3889: call to object of class type 'issue_857::o2::<lambda_1>': no matching call operator found pure2-last-use.cpp2(193): note: could be 'decltype(auto) issue_857::o2::<lambda_1>::operator ()(Obj &&,Params &&...) noexcept(<expr>) const' pure2-last-use.cpp2(193): note: the associated constraints are not satisfied pure2-last-use.cpp2(193): note: the constraint was not satisfied

Dropping the UFCS macro from CPP2_UFCS(std::move(*this).n)(0);
makes GCC and MSVC work: https://cpp1.godbolt.org/z/fWsWxbM34.
When lookup finds the member n, it won't continue past the type scope.
So we can just disable UFCS in this case: https://cpp2.godbolt.org/z/dax8Psaqj.

regression-tests/pure2-last-use.cpp2

Signed-off-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com>

MSVC error is resolved New Clang 12 error in `pure2-last-use.cpp2:938`

regression-tests/pure2-last-use.cpp2

hsutter · 2024-02-04T01:22:43Z

Whew! This is ~1100 lines, which I think makes it the biggest PR yet. Thanks for waiting for me to review it.

While I'm at it, clean up redundant headers that now we get from cpp2util.h And it's updating those regression test line-ends...

Signed-off-by: Herb Sutter <herb.sutter@gmail.com>

hsutter · 2024-02-04T01:46:54Z

@JohelEGP I think this is ready to merge. The only remaining thing is the three failing regression test runs, but those appear to be only because of diffs where the regression test baselines don't reflect the cpp2util.h line number changes, and so should be innocuous... does that sound right?

JohelEGP · 2024-02-04T01:49:18Z

That's right, according to the CI patches.
Thank you for reviewing.

hsutter · 2024-02-04T05:30:15Z

Thanks!

* fix(sema): recognize last use immediately after declaration * fix(sema): give function expression its own scope level * fix(sema): consider use as function in UFCS call * refactor(sema): remove stale comment * test(sema): also add unit test for `move` parameter * refactor(sema): avoid possible decrement beyond `begin` * fix(sema): move `this` on last use * test: update GCC 13 results * test: add Clang 18 results * test: add overloads with `that` * test: add another case that doesn't work yet * test: rename members to avoid shadowing * fix(sema): move implicit `this` of member on last use * test: remove commented cases that aren't last uses * fix(sema): guard check for `move this` parameter once * test: add case for destructor * test: add missing GCC 13 result * test: regenerate tests * fix(sema): consider use as function in UFCS call * fix(to_cpp1): type-scope variable initialization is not deferred * refactor(util): add UFCS macros to move and forward * fix(to_cpp1): emit last use of function in UFCS as part of macro name * test: regenerate GCC 13 result * fix(to_cpp1): exercise last use in range of for loop * refactor(util): remove UFCS macros of invalid combinations * refactor(to_cpp1): handle range of for loop specifically * refactor(to_cpp1): consider the expression list range * fix(sema): apply last use to call of member function * fix(sema): do not move a member function * fix(to_cpp1): do not move any member function * test: remove non-problematic comment about double move It just works, see <https://cpp1.godbolt.org/z/6fhjxrz65>. * refactor: regenerate `reflect.h` * refactor(sema): avoid decrementing before begin * fix(sema): extend implicit else branch to containing statement * fix(sema): increase scope to match an explicit else * refactor(sema): move heuristic to not change ending depths * fix(sema): pop consecutive implicit else branches * refactor(parse): put implicit else statements in the actual else node * test: add test cases that combine implicit and explicit access * test: add unit test for all test cases as selections * fix(reflect): fix initialization and regenerate * fix(sema): apply sema phase to generated code * test: regenerate results * test: add test case for another limitation * test: add another test case that fails * fix(sema): improve heuristic to only move from last (implicit) `this` * refactor(reflect): restore implicit `this` access * fix(sema): do not stack an implicit else branch * refactor: use "implicit scope" throughout * fix(sema): use local `move this` to determine last use * fix(sema): adjust condition for looking up to type * fix(sema): use last move on called type-scope variable * test: remove bogus comment * fix(sema): correct unset `this.` case * test: reference open issue * test: add test cases not affected by the open issue * test: clarify FIXME comment * test: review the new tests * perf: replace `std::map` with `mutable` data member * refactor: add `symbol::get_final_position` * refactor(to_cpp1): do not move single named return * test: add new test cases to commented out test case * test: add similar test case to callable case * refactor(sema): rework some outdated whitespace * test: regenerate results after "do not move single named return" * feat(sema): diagnose unused variables in generate code * fix(sema): consider variables in type scope declared later * test: refactor new test to use `f_copy` * refactor(sema): use member token * refactor(sema): update comments * refactor(sema): use the `this` parameter exclusively * refactor(sema): update the comments * refactor(sema): finish focusing on the implicit 'this' * fix(to_cpp1): move returned uninitialized local * fix(to_cpp1): move single named uninitialized return * test: add case with member declared last * refactor(sema): set condition for "at `this.`" correctly * fix(sema): use the better lookup algorithm for this case * fix(to_cpp1): stack current names only in namespaces * fix(to_cpp1): stack current names of bases * test: exercise forward in generated code * test: add stand-in for `std::move_only_function` * test: remove bogus test case * refactor(to_cpp1): rename to `is_class_member_access` * test: add test case showing limitations with base members * test: actually exercise forward in generated code * refactor(to_cpp1): reorder branch * refactor(to_cpp1): remove outdated condition * refactor(to_cpp1): rename to `is_class_member_access` * fix: revert using the empty implicit else branch Refs: e9cc033, 7f4a60f * fix(sema): change algorithm to find last uses * test: add test case for recursion logic * refactor(sema): simplify condition for UFCS on member * test: use `f_inout` and `f_copy` in earlier test cases * test: enable commented out tests * test: extend limitation unit test with alternative * test: remove redundant explicit discards * test: add more test cases for the new last use algorithm * test: add missing `new<int>()` * fix: regenerate `reflect.h` * test: add test cases of discovered problems * fix(sema): pop sibling branch conditions on found last use * refactor(sema): localize variables * fix(sema): recognize uses in iteration statements * fix(sema): start from the last symbol, not past-the-end * refactor(sema): add local type `pos_range` * fix(sema): handle loops and (non) captures * test: add similar case but without declaration * test: regenerate results * fix(reflect): update and regenerate `reflect.h` * fix(sema): start for loop at its body * refactor(sema): use `std::span` * refactor(sema): register name deactivation at visit * fix(sema): do not apply use to a hiding name * fix(sema): skip hiding loop parameter * test: revert `gcc-version.output` * fix(sema): recognize use in _next-clause_ * test: add corner case * fix(sema): recognize Cpp1 `using` declaration * refactor(sema): avoid adding duplicate symbols * refactor(sema): clarify similar members with comments * refactor(sema): turn comment into code * refactor(sema): modify local convenience variable * refactor(sema): remove expression scope * refactor(sema): use the right predicate * refactor(sema): remove inactive, stale assertions * refactor(sema): keep using a sentinel * fix(sema): handle a nested true branch * refactor(sema): revert whitespace change * refactor(sema): fix `started_postfix_expression` simpler * refactor(sema): revert stale fix of `scope_depth` * refactor(sema): comment the need of `final_position` at hand-out * refactor(to_cpp1): drop periods from comment * refactor(to_cpp1): reorder arguments for better formatting * refactor(to_cpp1): remove stale non-rvalue context * refactor(to_cpp1): remove useless non-rvalue context * refactor(to_cpp1): clarifiy comment with example * test: regenerate gcc-13 results Commit 4eef0da actually worked around hsutter#746. * test: regenerate results * refactor(sema): resolve CI issues * test: revert changes to gcc-13 result * refactor: generalize to `identifier_sym::safe_to_move` * test: fix implementation of `identity` * refactor(sema): add scoped indices of uses * refactor(sema): simplify names of activation entities * fix(sema): do not mark non-captures as captures * refactor(sema): rename to avoid verb prefix According to <hsutter#231 (comment)>: > to avoid dealing with English verb-to-adjective conventions * fix(sema): disable implicit move unsequenced with another use * fix(sema): consider _is-as-expression_ too * fix(sema): check all parameters for use * refactor(sema): remove wrong fix for UFCS issue * Minor updates to compile cppfront and the new test cleanly using MSVC And re-remove a few stray `;` that were removed as part of PR hsutter#911 and now cause errors because of the pedantic builds I regenerated `reflect.h` and found two changes that weren't already in the PR, so committing those too Also including the new test file's run against MSVC showing the five messages mentioned in the PR review, see PR review for more... * refactor(to_cpp1): pass `0` to `int` parameter instead of `false` * test: discard unused results * refactor(to_cpp1): avoid the UFCS macro to workaround compiler bugs * test: update GCC 13 results * test: remove Clang 18 results * refactor(sema): avoid pointer arithmetic * test: regenerate results * refactor(sema): avoid pointer arithmetic * Add regression test result diffs from my machine MSVC error is resolved New Clang 12 error in `pure2-last-use.cpp2:938` * test: comment Clang 12 limitation * test: apply CI patches * test: apply CI patches * test: apply CI patches * Tweak: Change "final position" to "global token order" * Minor tweaks While I'm at it, clean up redundant headers that now we get from cpp2util.h And it's updating those regression test line-ends... --------- Signed-off-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com> Signed-off-by: Herb Sutter <herb.sutter@gmail.com> Co-authored-by: Herb Sutter <herb.sutter@gmail.com>

JohelEGP added 6 commits December 12, 2023 01:17

fix(sema): recognize last use immediately after declaration

709b254

fix(sema): give function expression its own scope level

1cba29a

fix(sema): consider use as function in UFCS call

c014679

refactor(sema): remove stale comment

fc1fa52

test(sema): also add unit test for move parameter

286a32e

refactor(sema): avoid possible decrement beyond begin

e3e983b

JohelEGP changed the title ~~fix(sema): recognize actual uses broken by commit 243d03d3fd9608e0e76ddcbe8decfef9deb5ebdf~~ fix(sema): recognize uses of variables rejected as unused Dec 12, 2023

JohelEGP added 7 commits December 12, 2023 10:47

fix(sema): move this on last use

d79b5d7

test: update GCC 13 results

d0fd08d

test: add Clang 18 results

afb57e0

test: add overloads with that

a8e84ad

test: add another case that doesn't work yet

0315e1c

test: rename members to avoid shadowing

a05c514

fix(sema): move implicit this of member on last use

7f955f6

JohelEGP changed the title ~~fix(sema): recognize uses of variables rejected as unused~~ fix(sema): recognize more last uses, including of this parameter Dec 12, 2023

JohelEGP added 5 commits December 12, 2023 20:24

test: remove commented cases that aren't last uses

51c80ba

fix(sema): guard check for move this parameter once

257da26

test: add case for destructor

6397e20

test: add missing GCC 13 result

af66ec0

test: regenerate tests

d28828e

JohelEGP commented Dec 13, 2023

View reviewed changes

regression-tests/test-results/pure2-deducing-pointers-error.cpp2.output Show resolved Hide resolved

JohelEGP added 2 commits December 13, 2023 00:19

fix(sema): consider use as function in UFCS call

0a4ba35

fix(to_cpp1): type-scope variable initialization is not deferred

0ca5fdd

JohelEGP mentioned this pull request Dec 13, 2023

[BUG] Implicit move/forward interferes with main UFCS branch #888

Closed

JohelEGP added 6 commits December 13, 2023 07:17

refactor(util): add UFCS macros to move and forward

b212217

fix(to_cpp1): emit last use of function in UFCS as part of macro name

084fd4a

test: regenerate GCC 13 result

8da1f6d

fix(to_cpp1): exercise last use in range of for loop

fefe0da

refactor(util): remove UFCS macros of invalid combinations

545ea22

refactor(to_cpp1): handle range of for loop specifically

0004512

hsutter requested changes Jan 2, 2024

View reviewed changes

JohelEGP added 3 commits January 3, 2024 20:35

refactor(to_cpp1): pass 0 to int parameter instead of false

26d71c6

test: discard unused results

99f1556

refactor(to_cpp1): avoid the UFCS macro to workaround compiler bugs

cbd977b

JohelEGP requested a review from hsutter January 4, 2024 01:47

JohelEGP and others added 7 commits January 3, 2024 22:22

test: update GCC 13 results

241298e

test: remove Clang 18 results

8764fe4

refactor(sema): avoid pointer arithmetic

cc1516a

Merge branch 'main' into last_use

d97a328

Signed-off-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com>

test: regenerate results

d2d0d3e

refactor(sema): avoid pointer arithmetic

e3d17e2

Add regression test result diffs from my machine

80f499f

MSVC error is resolved New Clang 12 error in `pure2-last-use.cpp2:938`

hsutter reviewed Jan 25, 2024

View reviewed changes

regression-tests/pure2-last-use.cpp2 Outdated Show resolved Hide resolved

JohelEGP and others added 6 commits January 25, 2024 18:26

test: comment Clang 12 limitation

84edf89

Merge branch 'main' into last_use

782e078

test: apply CI patches

1b57ea0

test: apply CI patches

4382aa1

test: apply CI patches

7cf1093

Tweak: Change "final position" to "global token order"

870df49

hsutter approved these changes Feb 4, 2024

View reviewed changes

hsutter added 2 commits February 3, 2024 17:34

Minor tweaks

3b0f2c2

While I'm at it, clean up redundant headers that now we get from cpp2util.h And it's updating those regression test line-ends...

Merge branch 'main' into last_use

87c2344

Signed-off-by: Herb Sutter <herb.sutter@gmail.com>

hsutter merged commit 510eae8 into hsutter:main Feb 4, 2024
12 of 15 checks passed

JohelEGP deleted the last_use branch February 4, 2024 05:34

hsutter mentioned this pull request May 15, 2024

Feature/regular expression metafunction #904

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: recognize last uses (including of `this`) #887

fix: recognize last uses (including of `this`) #887

JohelEGP commented Dec 12, 2023 •

edited

Loading

hsutter left a comment

hsutter Jan 1, 2024

JohelEGP Jan 2, 2024

hsutter Jan 1, 2024

hsutter Jan 1, 2024

hsutter Jan 2, 2024

JohelEGP Jan 4, 2024

This comment was marked as resolved.

hsutter commented Feb 4, 2024

hsutter commented Feb 4, 2024 •

edited

Loading

JohelEGP commented Feb 4, 2024

hsutter commented Feb 4, 2024

fix: recognize last uses (including of this) #887

fix: recognize last uses (including of this) #887

Conversation

JohelEGP commented Dec 12, 2023 • edited Loading

hsutter left a comment

Choose a reason for hiding this comment

hsutter Jan 1, 2024

Choose a reason for hiding this comment

JohelEGP Jan 2, 2024

Choose a reason for hiding this comment

hsutter Jan 1, 2024

Choose a reason for hiding this comment

hsutter Jan 1, 2024

Choose a reason for hiding this comment

hsutter Jan 2, 2024

Choose a reason for hiding this comment

JohelEGP Jan 4, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

hsutter commented Feb 4, 2024

hsutter commented Feb 4, 2024 • edited Loading

JohelEGP commented Feb 4, 2024

hsutter commented Feb 4, 2024

fix: recognize last uses (including of `this`) #887

fix: recognize last uses (including of `this`) #887

JohelEGP commented Dec 12, 2023 •

edited

Loading

hsutter commented Feb 4, 2024 •

edited

Loading