Skip to content

Commit

Permalink
Work around a C++20 compiler's range-for scoped variable bug/ICE
Browse files Browse the repository at this point in the history
  • Loading branch information
@hsutter
hsutter committed May 7, 2023
1 parent 4d034f3 commit 2c64707
Show file tree
Hide file tree
Showing 16 changed files with 72 additions and 49 deletions.
10 changes: 6 additions & 4 deletions regression-tests/test-results/mixed-bounds-safety-with-assert-2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ auto add_42_to_subrange(auto& rng, cpp2::in<int> start, cpp2::in<int> end) -> vo
std::vector<int> v {1, 2, 3, 4, 5};
add_42_to_subrange(v, 1, 3);

for ( auto const& cpp2_range = v; auto const& i : cpp2_range )
std::cout << i << "\n";
{ auto const& cpp2_range = v; for ( auto const& i : cpp2_range )
std::cout << i << "\n";}
#line 8 "mixed-bounds-safety-with-assert-2.cpp2"
}

auto add_42_to_subrange(auto& rng, cpp2::in<int> start, cpp2::in<int> end) -> void
Expand All @@ -41,11 +42,12 @@ auto add_42_to_subrange(auto& rng, cpp2::in<int> start, cpp2::in<int> end) -> vo
cpp2::Bounds.expects(cpp2::cmp_less_eq(end,CPP2_UFCS_0(ssize, rng)), "");

auto count {0};
for ( auto&& cpp2_range = rng;
{ auto&& cpp2_range = rng; for (

auto& i : cpp2_range ) { do
if ([_0 = start, _1 = count, _2 = end]{ return cpp2::cmp_less_eq(_0,_1) && cpp2::cmp_less_eq(_1,_2); }()) {
i += 42;
} while (false); ++count; }
} while (false); ++count; }}
#line 22 "mixed-bounds-safety-with-assert-2.cpp2"
}

5 changes: 3 additions & 2 deletions regression-tests/test-results/mixed-bounds-safety-with-assert.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,12 @@ auto print_subrange(auto const& rng, cpp2::in<int> start, cpp2::in<int> end) ->
cpp2::Bounds.expects(cpp2::cmp_less_eq(end,CPP2_UFCS_0(ssize, rng)), "");

auto count {0};
for ( auto const& cpp2_range = rng;
{ auto const& cpp2_range = rng; for (

auto const& i : cpp2_range ) { do
if (cpp2::cmp_less_eq(start,count) && cpp2::cmp_less_eq(count,end)) {
std::cout << i << "\n";
} while (false); ++count; }
} while (false); ++count; }}
#line 18 "mixed-bounds-safety-with-assert.cpp2"
}

5 changes: 3 additions & 2 deletions regression-tests/test-results/mixed-fixed-type-aliases.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ auto test(auto const& x) -> void{
cpp2::u16 z {42};
test(std::move(z));

for ( auto const& cpp2_range = args; auto const& arg : cpp2_range )
std::cout << arg << "\n";
{ auto const& cpp2_range = args; for ( auto const& arg : cpp2_range )
std::cout << arg << "\n";}
#line 24 "mixed-fixed-type-aliases.cpp2"
}

5 changes: 3 additions & 2 deletions regression-tests/test-results/mixed-function-expression-and-std-for-each.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@
std::move(callback)
);

for ( auto const& cpp2_range = view; auto const& str : cpp2_range ) {
{ auto const& cpp2_range = view; for ( auto const& str : cpp2_range ) {
std::cout << str << "\n";
}
}}
#line 30 "mixed-function-expression-and-std-for-each.cpp2"
}

5 changes: 3 additions & 2 deletions ...ion-tests/test-results/mixed-function-expression-and-std-ranges-for-each-with-capture.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
auto callback {[](auto& x) -> void { x += "-ish"; }};
std::ranges::for_each(view, std::move(callback));

for ( auto const& cpp2_range = view; auto const& str : cpp2_range )
std::cout << str << "\n";
{ auto const& cpp2_range = view; for ( auto const& str : cpp2_range )
std::cout << str << "\n";}
#line 22 "mixed-function-expression-and-std-ranges-for-each-with-capture.cpp2"
}

5 changes: 3 additions & 2 deletions regression-tests/test-results/mixed-function-expression-and-std-ranges-for-each.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@
auto callback {[](auto& x) -> void { x += "-ish"; }};
std::ranges::for_each(view, std::move(callback));

for ( auto const& cpp2_range = view; auto const& str : cpp2_range )
std::cout << str << "\n";
{ auto const& cpp2_range = view; for ( auto const& str : cpp2_range )
std::cout << str << "\n";}
#line 21 "mixed-function-expression-and-std-ranges-for-each.cpp2"
}

5 changes: 3 additions & 2 deletions regression-tests/test-results/mixed-function-expression-with-pointer-capture.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
auto callback {[](auto& x) -> void { x += "-ish"; }};
std::ranges::for_each(view, std::move(callback));

for ( auto const& cpp2_range = view; auto const& str : cpp2_range )
std::cout << str << "\n";
{ auto const& cpp2_range = view; for ( auto const& str : cpp2_range )
std::cout << str << "\n";}
#line 23 "mixed-function-expression-with-pointer-capture.cpp2"
}

5 changes: 3 additions & 2 deletions regression-tests/test-results/mixed-function-expression-with-repeated-capture.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
auto callback {[](auto& x) -> void { x += "-ish"; }};
std::ranges::for_each(view, std::move(callback));

for ( auto const& cpp2_range = view; auto const& str : cpp2_range )
std::cout << str << "\n";
{ auto const& cpp2_range = view; for ( auto const& str : cpp2_range )
std::cout << str << "\n";}
#line 22 "mixed-function-expression-with-repeated-capture.cpp2"
}

5 changes: 3 additions & 2 deletions regression-tests/test-results/mixed-intro-for-with-counter-include-last.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@
{
std::vector<int> v {1, 2, 3, 4, 5};
auto counter {42};
for ( auto const& cpp2_range = v; auto const& i : cpp2_range ) { do {
{ auto const& cpp2_range = v; for ( auto const& i : cpp2_range ) { do {
std::cout << i << " " << counter << "\n";
} while (false); counter *= 2; }
} while (false); counter *= 2; }}
#line 9 "mixed-intro-for-with-counter-include-last.cpp2"
}

36 changes: 20 additions & 16 deletions regression-tests/test-results/pure2-break-continue.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,83 +253,87 @@ auto for_continue_inner() -> void
{
std::vector vi {0, 1, 2};
auto counter {0};
for ( auto const& cpp2_range = vi; auto const& i : cpp2_range ) { do {
{ auto const& cpp2_range = vi; for ( auto const& i : cpp2_range ) { do {
std::vector vj {0, 1, 2};
for ( auto const& cpp2_range = vj; auto const& j : cpp2_range ) {
{ auto const& cpp2_range = vj; for ( auto const& j : cpp2_range ) {
#line 166 "pure2-break-continue.cpp2"
{
std::cout << i << j << " ";
if (j==1) {
goto CONTINUE_166_9;
}
std::cout << "inner ";
} CPP2_CONTINUE_BREAK(166_9) }
} CPP2_CONTINUE_BREAK(166_9) }}

#line 174 "pure2-break-continue.cpp2"
std::cout << "outer ";
} while (false); ++counter; }
} while (false); ++counter; }}
#line 176 "pure2-break-continue.cpp2"
}

auto for_continue_outer() -> void
{
std::vector vi {0, 1, 2};
auto counter {0};
for ( auto const& cpp2_range = vi; auto const& i : cpp2_range ) {
{ auto const& cpp2_range = vi; for ( auto const& i : cpp2_range ) {
#line 182 "pure2-break-continue.cpp2"
{ do {
std::vector vj {0, 1, 2};
for ( auto const& cpp2_range = vj; auto const& j : cpp2_range ) {
{ auto const& cpp2_range = vj; for ( auto const& j : cpp2_range ) {
std::cout << i << j << " ";
if (j==1) {
goto CONTINUE_182_5;
}
std::cout << "inner ";
}
}}

#line 192 "pure2-break-continue.cpp2"
std::cout << "outer ";
} while (false); ++counter; } CPP2_CONTINUE_BREAK(182_5) }
} while (false); ++counter; } CPP2_CONTINUE_BREAK(182_5) }}
#line 194 "pure2-break-continue.cpp2"
}

auto for_break_inner() -> void
{
std::vector vi {0, 1, 2};
auto counter {0};
for ( auto const& cpp2_range = vi; auto const& i : cpp2_range ) { do {
{ auto const& cpp2_range = vi; for ( auto const& i : cpp2_range ) { do {
std::vector vj {0, 1, 2};
for ( auto const& cpp2_range = vj; auto const& j : cpp2_range ) {
{ auto const& cpp2_range = vj; for ( auto const& j : cpp2_range ) {
#line 202 "pure2-break-continue.cpp2"
{
std::cout << i << j << " ";
if (j==1) {
goto BREAK_202_9;
}
std::cout << "inner ";
} CPP2_CONTINUE_BREAK(202_9) }
} CPP2_CONTINUE_BREAK(202_9) }}

#line 210 "pure2-break-continue.cpp2"
std::cout << "outer ";
} while (false); ++counter; }
} while (false); ++counter; }}
#line 212 "pure2-break-continue.cpp2"
}

auto for_break_outer() -> void
{
std::vector vi {0, 1, 2};
auto counter {0};
for ( auto const& cpp2_range = vi; auto const& i : cpp2_range ) {
{ auto const& cpp2_range = vi; for ( auto const& i : cpp2_range ) {
#line 218 "pure2-break-continue.cpp2"
{ do {
std::vector vj {0, 1, 2};
for ( auto const& cpp2_range = vj; auto const& j : cpp2_range ) {
{ auto const& cpp2_range = vj; for ( auto const& j : cpp2_range ) {
std::cout << i << j << " ";
if (j==1) {
goto BREAK_218_5;
}
std::cout << "inner ";
}
}}

#line 228 "pure2-break-continue.cpp2"
std::cout << "outer ";
} while (false); ++counter; } CPP2_CONTINUE_BREAK(218_5) }
} while (false); ++counter; } CPP2_CONTINUE_BREAK(218_5) }}
#line 230 "pure2-break-continue.cpp2"
}

5 changes: 3 additions & 2 deletions regression-tests/test-results/pure2-intro-example-hello-2022.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ auto println(auto const& x, auto const& len) -> void;
"hello", "2022"};
std::span view {vec};

for ( auto&& cpp2_range = view; auto& str : cpp2_range ) {
{ auto&& cpp2_range = view; for ( auto& str : cpp2_range ) {
auto len {decorate(str)};
println(str, len);
}
}}
#line 10 "pure2-intro-example-hello-2022.cpp2"
}

[[nodiscard]] auto decorate(auto& thing) -> int{
Expand Down
5 changes: 3 additions & 2 deletions regression-tests/test-results/pure2-intro-example-three-loops.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,10 @@ auto decorate_and_print(auto& thing) -> void{
} while ( cpp2::cmp_greater(*cpp2::assert_not_null(i),1) && [&]{ --*cpp2::assert_not_null(i) ; return true; }() );

std::cout << "\n";
for ( auto&& cpp2_range = words; auto& word : cpp2_range )
decorate_and_print(word);
{ auto&& cpp2_range = words; for ( auto& word : cpp2_range )
decorate_and_print(word);}

#line 28 "pure2-intro-example-three-loops.cpp2"
print(std::string{"end of program"});
}

4 changes: 2 additions & 2 deletions regression-tests/test-results/pure2-statement-scope-parameters.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ auto const& i = local_int;

// 'in' (read-only) statement scope variable
#line 6 "pure2-statement-scope-parameters.cpp2"
for ( auto const& cpp2_range = args; auto const& arg : cpp2_range ) {
{ auto const& cpp2_range = args; for ( auto const& arg : cpp2_range ) {
std::cout << i << "\n"; // prints 42
}
}}
}
{
auto& i = local_int;
Expand Down
5 changes: 3 additions & 2 deletions regression-tests/test-results/pure2-type-and-namespace-aliases.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,9 @@ auto myfunc() -> void{

auto const& v2 = std::move(v);

for ( auto const& cpp2_range = v2; auto const& s : cpp2_range )
std::cout << cpp2::to_string(s) + "\n";
{ auto const& cpp2_range = v2; for ( auto const& s : cpp2_range )
std::cout << cpp2::to_string(s) + "\n";}
#line 24 "pure2-type-and-namespace-aliases.cpp2"
}

auto main() -> int{
Expand Down
2 changes: 1 addition & 1 deletion regression-tests/test-results/version
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

cppfront compiler v0.2.1 Build 8505:0902
cppfront compiler v0.2.1 Build 8506:1618
Copyright(c) Herb Sutter All rights reserved

SPDX-License-Identifier: CC-BY-NC-ND-4.0
Expand Down
14 changes: 10 additions & 4 deletions source/cppfront.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2116,14 +2116,18 @@ class cppfront
&& n.body
);

// Note: This used to emit cpp2_range as a range-for-loop scope variable,
// but some major compilers seem to have random troubles with that;
// the workaround to avoid their bugs for now is to emit a { } block
// around the Cpp1 range-for and make the scope variable a normal local
if (n.for_with_in) {
printer.print_cpp2("for ( auto const& cpp2_range = ", n.position());
printer.print_cpp2("{ auto const& cpp2_range = ", n.position());
}
else {
printer.print_cpp2("for ( auto&& cpp2_range = ", n.position());
printer.print_cpp2("{ auto&& cpp2_range = ", n.position());
}
emit(*n.range);
printer.print_cpp2("; ", n.position());
printer.print_cpp2("; for ( ", n.position());
emit(*n.parameter);
printer.print_cpp2(" : cpp2_range ) ", n.position());
if (!labelname.empty()) {
Expand Down Expand Up @@ -2151,6 +2155,8 @@ class cppfront
printer.print_extra(" CPP2_CONTINUE_BREAK("+labelname+") }");
}

printer.print_extra("}");

in_non_rvalue_context.pop_back();
}

Expand Down Expand Up @@ -5410,7 +5416,7 @@ class cppfront
{
printer.print_cpp2( prefix, n.position() );
printer.print_cpp2( "auto " + type_qualification_if_any_for(n), n.position() );
printer.print_cpp2( *n.name(), n.identifier->position() );
emit( *n.name() );
emit( *func, n.name(), is_main, false, suffix1 );
printer.print_cpp2( suffix2, n.position() );
}
Expand Down

4 comments on commit 2c64707

@JohelEGP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that this interacts with
1683483241
1683483251

@hsutter
Copy link
Owner Author

@hsutter hsutter commented on 2c64707 May 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good point. The major compiler's confusion/ICE about the range-for scope variable was unpleasant though, and I hit it half a dozen times with just the loops in reflect.h.

That said, one reason I even give the range a name is to make it const& if it's a for loop with an in parameter...

            if (n.for_with_in) {
                printer.print_cpp2("{ auto const& cpp2_range = ", n.position());
            }
            else {
                printer.print_cpp2("{ auto&& cpp2_range = ", n.position());
            }

In #234, I had tried using as_const but hit the very lifetime bug that P2644 and P2718 fix...

Update: On rereading P2718, I don't think that paper affects this commit, because IIUC P2718 applies the lifetime extension narrowly to just the for-range-initializer, not to the loop scope variable. BTW, this kind of thing is why many people wanted the fix in P2718 to apply to all lifetime extension.

@JohelEGP
Copy link
Contributor

@JohelEGP JohelEGP commented on 2c64707 May 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, one reason I even give the range a name is to make it const& if it's a for loop with an in parameter...

I see.
But the constness of the range expression and loop variable should be independent.
That can be impactful when it comes to non-const ranges.
https://cpp2.godbolt.org/z/vvWvqdGdh can't work
(it seems there's a cppfront bug [now #432], so checkout https://compiler-explorer.com/z/o4vzTK7cG).

I think it'd be better to, fixing #386 (comment) (just accidentally deleted, recovered what I could at #386 (comment)), have these semantics:

So, I'd say, in the happy path:

The Cpp2 range-based for statement

label? 'for' expression next-clause? 'do' unnamed-declaration

is equivalent to (after lowering to Cpp1)

{
  label?
  for ( parameter-declaration : expression ) {
    { compound-statement }
    next-clause?
  }
}

where parameter-declaration and compound-statement
are the immediate productions that make up the unnamed-declaration.

So Cpp2

for range do (in x) f(x);
(y := 0) for range do (in x) f(x);

lowers to Cpp1

for (const auto& x: range) f(x);
for (const auto& y = 0; const auto& x: range) f(x);

@hsutter
Copy link
Owner Author

@hsutter hsutter commented on 2c64707 May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, the more I look at this, the more I can't see why this wants a separate "range" variable. Just putting the range right after the Cpp1 : is simpler code, and will benefit from the C++23 lifetime extension too. You're right, thanks!

I've fixed this in commit 028caa0.

Re this though:

(y := 0) for range do (in x) f(x);

[...] lowers to Cpp1 [...]

for (const auto& y = 0; const auto& x: range) f(x);

In Cpp1 this only works for a single variable. So I like the general way that we currently do this for N variables for any statement (and that also works for for).

Again, thanks.

Please sign in to comment.