Page MenuHomePhabricator

[clang-format] [PR35518] C++17 deduction guides are wrongly formatted
ClosedPublic

Authored by MyDeveloperDay on Oct 29 2019, 11:42 AM.

Details

Summary

see https://bugs.llvm.org/show_bug.cgi?id=35518

clang-format removes spaces around deduction guides but not trailing return types, make the consistent

template <typename T> S(T)->S<T>;
auto f(int, int) -> double;

becomes

template <typename T> S(T) -> S<T>;
auto f(int, int) -> double;

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2019, 11:42 AM

Should we find a way to set ->'s type to TT_TrailingReturnArrow?

Should we find a way to set ->'s type to TT_TrailingReturnArrow?

that's possible then we might be able to use the existing spaces before rule

if (Right.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow) ||
    Left.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow))
  return true;
MyDeveloperDay added a reviewer: lichray.

Detect deduction guides arrow as a TrailingReturn arrow, allowing use of existing space before/after rules

klimek added inline comments.Oct 30 2019, 9:34 AM
clang/lib/Format/TokenAnnotator.cpp
1464–1467

Why doesn't this trigger on function templates:

c()->f<int>();
lichray added inline comments.Oct 30 2019, 9:46 AM
clang/lib/Format/TokenAnnotator.cpp
1464

Comparing to the else if branch above, several questions can arise:

  1. Has deduction-guide be considered a declaration (it is, of course, in standard)? If yes, without MustBeDeclaration, how x = p->foo<3>(); being formatted?
  2. Without restrictions on NestingLevel, how A() -> A<decltype(p->foo<3>())>; being formatted?
  3. How x()->foo<1>; being formatted? What's the difference between this and a deduction-guide? A deduction-guide has to follow TheSameType(...) -> TheSameType<....>; and appears only at namespace level, do these help?

Oh no, auto x = p -> foo<1>(); this is a bug (I will look for bug reports, don't mind).

MyDeveloperDay marked an inline comment as done.Oct 30 2019, 10:01 AM
MyDeveloperDay added a subscriber: hans.
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
1464–1467

This case I agree is wrong (but that comes from the existing rule no?)

auto x = p -> foo<1>();

The other examples are currently as follows

c()->f<int>();
x()->foo<1>;
x = p->foo<3>();
A()->A<decltype(p->foo<3>())>;

This is how they look from the last build @hans made on the 17th October

auto x = p -> foo<1>();
c()->f<int>();
x()->foo<1>;
x = p->foo<3>();
A()->A<decltype(p->foo<3>())>;

Debug info

----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=auto L=4 PPK=2 FakeLParens=2/ FakeRParens=0 II=0x2c52129f718 Text='auto'
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=220 Name=identifier L=6 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2da8 Text='x'
 M=0 C=0 T=BinaryOperator S=1 B=0 BK=0 P=22 Name=equal L=8 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='='
 M=0 C=1 T=Unknown S=1 B=0 BK=0 P=22 Name=identifier L=10 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2dd8 Text='p'
 M=0 C=1 T=TrailingReturnArrow S=1 B=0 BK=0 P=23 Name=arrow L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=23 Name=identifier L=17 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d78 Text='foo'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=18 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=numeric_constant L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='1'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=20 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=l_paren L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=59 Name=r_paren L=22 PPK=2 FakeLParens= FakeRParens=2 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=23 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2e08 Text='c'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=2 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=140 Name=r_paren L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=170 Name=arrow L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=TrailingAnnotation S=0 B=0 BK=0 P=190 Name=identifier L=6 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d48 Text='f'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=int L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x2c52129fa50 Text='int'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=l_paren L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=140 Name=r_paren L=13 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2da8 Text='x'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=2 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=140 Name=r_paren L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=170 Name=arrow L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=TrailingAnnotation S=0 B=0 BK=0 P=190 Name=identifier L=8 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d78 Text='foo'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=9 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=numeric_constant L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='1'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=11 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=semi L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens=2/ FakeRParens=0 II=0x2c5212a2da8 Text='x'
 M=0 C=0 T=BinaryOperator S=1 B=0 BK=0 P=22 Name=equal L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='='
 M=0 C=1 T=Unknown S=1 B=0 BK=0 P=22 Name=identifier L=5 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2dd8 Text='p'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=170 Name=arrow L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=identifier L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d78 Text='foo'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=numeric_constant L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='3'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=l_paren L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=59 Name=r_paren L=15 PPK=2 FakeLParens= FakeRParens=2 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=16 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2cb8 Text='A'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=2 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=140 Name=r_paren L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=170 Name=arrow L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=TrailingAnnotation S=0 B=0 BK=0 P=190 Name=identifier L=6 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2cb8 Text='A'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=decltype L=15 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a09f8 Text='decltype'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=263 Name=l_paren L=16 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=380 Name=identifier L=17 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2dd8 Text='p'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=430 Name=arrow L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=283 Name=identifier L=22 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d78 Text='foo'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=290 Name=less L=23 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=620 Name=numeric_constant L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='3'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=530 Name=greater L=25 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=290 Name=l_paren L=26 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=400 Name=r_paren L=27 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=283 Name=r_paren L=28 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=29 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=semi L=30 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

I should mention all existing unit tests pass, Beyonce rule!

MyDeveloperDay added a comment.EditedOct 30 2019, 10:02 AM

Sorry ignore my last results, let me rerun, I was using the revision without the change.

Here is what they'll look like:

auto x = p -> foo<1>();
c() -> f<int>();
x() -> foo<1>;
x = p->foo<3>();
A() -> A<decltype(p->foo<3>())>;
lichray added inline comments.Oct 30 2019, 10:11 AM
clang/lib/Format/TokenAnnotator.cpp
1464–1467

move detection of deduction guides into a function, add additional negative tests

MyDeveloperDay marked 4 inline comments as done.Oct 30 2019, 1:02 PM
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
1464–1467

let us cover that separately, as the logic for that is in the else if clause above and the bug is present prior to this revision.

lichray added inline comments.Oct 30 2019, 1:38 PM
clang/lib/Format/TokenAnnotator.cpp
1354

Parentheses not matching is comment.

1365

Does this work? What about A() -> A<(3 < 2)>;?

1366

What's this for? What about A() -> A<sizeof(p->foo<1>>);

I guess we don't have to look for the end of template, because a class X can't refer to its member called X with -> because that member is constructor. The approach to match the identifiers may not work in the future but may work fine for now.

clang/unittests/Format/FormatTest.cpp
4992

This looks like a deduction guide. It should be formatted as A() -> A<decltype(p->foo<3>())>; I assume? If you cannot cover this case in this patch, this test case needs a comment.

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay set the repository for this revision to rC Clang.

Address review comments, deduction guides with embedded parens

MyDeveloperDay marked 4 inline comments as done.Oct 31 2019, 2:08 AM

The functionality looks acceptable. Trying to parse the whole thing still looks fragile to me. I expect code owner to take a look at this change.

clang/lib/Format/TokenAnnotator.cpp
1371

Maybe make use of some TT_TemplateOpener?

clang/unittests/Format/FormatTest.cpp
4987

Does A() -> A<decltype(foo<traits<1>>)> (C++11 >>) work?

MyDeveloperDay marked 4 inline comments as done.

Add additional test case

clang/lib/Format/TokenAnnotator.cpp
1371

We can't use TT_TemplateOpener because like MatchParen it hasn't been set yet on the downstream tokens

clang/unittests/Format/FormatTest.cpp
4987

this should work because we are skipping everything in between the (....)

Minor comments.

clang/lib/Format/TokenAnnotator.cpp
1354

Please write full-phrase comments.

clang/unittests/Format/FormatTest.cpp
4995

What about:

verifyFormat("x()->x<1>;");

i.e. a function x returning a pointer to a class having a template member x (for instance a template variable).

MyDeveloperDay marked 2 inline comments as done.Nov 4 2019, 1:58 PM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
4995

I'm unsure if we are the limit of being able to differentiate between the two?

curdeius added inline comments.Nov 5 2019, 12:44 AM
clang/unittests/Format/FormatTest.cpp
4995

Well, deduction guides always have a template parameter list before class name (unless I'm mistaken), so we should be able to distinguish the two.

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay set the repository for this revision to rG LLVM Github Monorepo.

Ensure the deduction guides follow a template
Add additional test cases raised during the reviews

curdeius accepted this revision.Nov 5 2019, 1:18 PM

LGTM. Just a small typo in comment. Otherwise, great job!

clang/lib/Format/TokenAnnotator.cpp
1369

Typo: template.

This revision is now accepted and ready to land.Nov 5 2019, 1:18 PM

Build result: pass - 59867 tests passed, 0 failed and 768 were skipped.
Log files: console-log.txt, CMakeCache.txt

MyDeveloperDay marked 3 inline comments as done.Nov 6 2019, 12:33 AM
This revision was automatically updated to reflect the committed changes.