Page MenuHomePhabricator

[clang-format] broken after lambda with return type template with boolean literal
ClosedPublic

Authored by MyDeveloperDay on Mar 4 2019, 12:44 PM.

Diff Detail

Repository
rC Clang

Event Timeline

MyDeveloperDay created this revision.Mar 4 2019, 12:44 PM
jkorous added a subscriber: jkorous.Mar 4 2019, 1:47 PM

Thanks for fixing this!

The patch LGTM.

I am just wondering what else are we missing here. I guess arithmetic and logical operators are also allowed:

template<int> struct foo {};

int main() { 
    auto lambda1 = []() -> foo<!1> { return foo<!1>{}; }; 
    auto lambda2 = []() -> foo<5+3> { return foo<5+3>{}; }; 
}

I added couple other cases to your fix here: https://reviews.llvm.org/D58934

BTW I suggest you also add the original test case since the test cases you added fail the expectation at FormatTest.cpp:74 and the message is a bit unclear whether the testcase or the actual implementation is at fault.

EXPECT_EQ(Expected.str(), format(Expected, Style))
    << "Expected code is not stable";

The original reported case IMO gives a better idea what is broken since it duplicates the end-of-namespace comment.
https://bugs.llvm.org/show_bug.cgi?id=40910

BTW I suggest you also add the original test case since the test cases you added fail the expectation at FormatTest.cpp:74 and the message is a bit unclear whether the testcase or the actual implementation is at fault.

EXPECT_EQ(Expected.str(), format(Expected, Style))
    << "Expected code is not stable";

The original reported case IMO gives a better idea what is broken since it duplicates the end-of-namespace comment.
https://bugs.llvm.org/show_bug.cgi?id=40910

do you mean this case? as this seems to work for me?

verifyFormat("namespace bar {\n"
               "auto foo{[]() -> foo<false> { ; }};\n"
               "} // namespace bar");
$ ./tools/clang/unittests/Format/FormatTests.exe --gtest_filter=FormatTest.FormatsLambdas*
Note: Google Test filter = FormatTest.FormatsLambdas*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.FormatsLambdas
[       OK ] FormatTest.FormatsLambdas (1353 ms)
[----------] 1 test from FormatTest (1354 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1362 ms total)
[  PASSED  ] 1 test.

BTW if you want me to land this before you land D58934: [clang-format] Fix lambdas returning template specialization that contains operator in parameter I need someone to mark this as accepted in Phabricator.

jkorous accepted this revision.Mar 5 2019, 11:19 AM

do you mean this case? as this seems to work for me?

verifyFormat("namespace bar {\n"
               "auto foo{[]() -> foo<false> { ; }};\n"
               "} // namespace bar");

I meant that if you take the test and run it on unpatched master (simulating hypothetical test failure in the future) the message it produces starts by stating that the expected string (part of the testcase) is "unstable" which seems a bit unclear/confusing to me.

/Users/jankorous/src/llvm-monorepo/llvm-project/clang/unittests/Format/FormatTest.cpp:74: Failure
      Expected: Expected.str()
      Which is: "namespace bar {\nauto foo{[]() -> foo<false> { ; }};\n} // namespace bar"
To be equal to: format(Expected, Style)
      Which is: "namespace bar {\nauto foo{[]() -> foo<false>{;\n}\n}\n;\n} // namespace bar"
With diff:
@@ -1,3 +1,6 @@
 namespace bar {
-auto foo{[]() -> foo<false> { ; }};
+auto foo{[]() -> foo<false>{;
+}
+}
+;
 } // namespace bar

Whereas the full original reproducer (with the "// broken:" comment included) also starts with the same message but the diff implies that the implementation is wrong by duplicating the "// namespace bar" comment.

/Users/jankorous/src/llvm-monorepo/llvm-project/clang/unittests/Format/FormatTest.cpp:74: Failure
      Expected: Expected.str()
      Which is: "namespace bar {\n// broken:\nauto foo{[]() -> foo<5 + 2> { return {}; }};\n} // namespace bar"
To be equal to: format(Expected, Style)
      Which is: "namespace bar {\n// broken:\nauto foo{[]() -> foo<5 + 2>{return {};\n} // namespace bar\n}\n;\n} // namespace bar"
With diff:
@@ -1,4 +1,7 @@
 namespace bar {
 // broken:
-auto foo{[]() -> foo<5 + 2> { return {}; }};
+auto foo{[]() -> foo<5 + 2>{return {};
+} // namespace bar
+}
+;
 } // namespace bar

Anyway. LGTM.

This revision is now accepted and ready to land.Mar 5 2019, 11:19 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 2:19 PM