[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Emilia Dreamer via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc70e360b355a: [clang-format] Allow trailing return types in 
macros (authored by rymiel).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8010,6 +8010,11 @@
"auto aa(T t)\n"
"-> decltype(eaaa(t.a).());");
 
+  FormatStyle Style = getLLVMStyleWithColumns(60);
+  verifyFormat("#define MAKE_DEF(NAME) 
\\\n"
+   "  auto NAME() -> int { return 42; }",
+   Style);
+
   // Not trailing return types.
   verifyFormat("void f() { auto a = b->c(); }");
   verifyFormat("auto a = p->foo();");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1909,7 +1909,8 @@
 } else if (Current.is(tok::arrow) &&
Style.Language == FormatStyle::LK_Java) {
   Current.setType(TT_LambdaArrow);
-} else if (Current.is(tok::arrow) && AutoFound && Line.MustBeDeclaration &&
+} else if (Current.is(tok::arrow) && AutoFound &&
+   (Line.MustBeDeclaration || Line.InPPDirective) &&
Current.NestingLevel == 0 &&
!Current.Previous->isOneOf(tok::kw_operator, tok::identifier)) {
   // not auto operator->() -> xxx;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8010,6 +8010,11 @@
"auto aa(T t)\n"
"-> decltype(eaaa(t.a).());");
 
+  FormatStyle Style = getLLVMStyleWithColumns(60);
+  verifyFormat("#define MAKE_DEF(NAME) \\\n"
+   "  auto NAME() -> int { return 42; }",
+   Style);
+
   // Not trailing return types.
   verifyFormat("void f() { auto a = b->c(); }");
   verifyFormat("auto a = p->foo();");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1909,7 +1909,8 @@
 } else if (Current.is(tok::arrow) &&
Style.Language == FormatStyle::LK_Java) {
   Current.setType(TT_LambdaArrow);
-} else if (Current.is(tok::arrow) && AutoFound && Line.MustBeDeclaration &&
+} else if (Current.is(tok::arrow) && AutoFound &&
+   (Line.MustBeDeclaration || Line.InPPDirective) &&
Current.NestingLevel == 0 &&
!Current.Previous->isOneOf(tok::kw_operator, tok::identifier)) {
   // not auto operator->() -> xxx;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Emilia Dreamer via Phabricator via cfe-commits]

rymiel updated this revision to Diff 507451.
rymiel added a comment.

Reduce column limit for macro test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8010,6 +8010,11 @@
"auto aa(T t)\n"
"-> decltype(eaaa(t.a).());");
 
+  FormatStyle Style = getLLVMStyleWithColumns(60);
+  verifyFormat("#define MAKE_DEF(NAME) 
\\\n"
+   "  auto NAME() -> int { return 42; }",
+   Style);
+
   // Not trailing return types.
   verifyFormat("void f() { auto a = b->c(); }");
   verifyFormat("auto a = p->foo();");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1909,7 +1909,8 @@
 } else if (Current.is(tok::arrow) &&
Style.Language == FormatStyle::LK_Java) {
   Current.setType(TT_LambdaArrow);
-} else if (Current.is(tok::arrow) && AutoFound && Line.MustBeDeclaration &&
+} else if (Current.is(tok::arrow) && AutoFound &&
+   (Line.MustBeDeclaration || Line.InPPDirective) &&
Current.NestingLevel == 0 &&
!Current.Previous->isOneOf(tok::kw_operator, tok::identifier)) {
   // not auto operator->() -> xxx;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8010,6 +8010,11 @@
"auto aa(T t)\n"
"-> decltype(eaaa(t.a).());");
 
+  FormatStyle Style = getLLVMStyleWithColumns(60);
+  verifyFormat("#define MAKE_DEF(NAME) \\\n"
+   "  auto NAME() -> int { return 42; }",
+   Style);
+
   // Not trailing return types.
   verifyFormat("void f() { auto a = b->c(); }");
   verifyFormat("auto a = p->foo();");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1909,7 +1909,8 @@
 } else if (Current.is(tok::arrow) &&
Style.Language == FormatStyle::LK_Java) {
   Current.setType(TT_LambdaArrow);
-} else if (Current.is(tok::arrow) && AutoFound && Line.MustBeDeclaration &&
+} else if (Current.is(tok::arrow) && AutoFound &&
+   (Line.MustBeDeclaration || Line.InPPDirective) &&
Current.NestingLevel == 0 &&
!Current.Previous->isOneOf(tok::kw_operator, tok::identifier)) {
   // not auto operator->() -> xxx;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Owen Pan via Phabricator via cfe-commits]

owenpan accepted this revision.
owenpan added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:7989-7990
 
+  verifyFormat("#define MAKE_DEF(NAME) 
"
+   "\\\n"
+   "  auto NAME() -> int { return 42; }");

I'd reduce `ColumnLimit` to 60 for example to get rid of the string 
concatenation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Emilia Dreamer via Phabricator via cfe-commits]

rymiel requested review of this revision.
rymiel added a comment.
This revision is now accepted and ready to land.

Okay, I planned changes because I had more ambitious plans for fixing this, but 
those didn't work out, so instead I made a separate issue 
(https://github.com/llvm/llvm-project/issues/61469).
This patch in its current state is a simple fix, so it's probably fine as is


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D141811#4070629 , @rymiel wrote:

> In D141811#4068180 , 
> @HazardyKnusperkeks wrote:
>
>> I thought about `auto NAME() -> int { return 42; }`.
>>
>> `decltype(auto) a = (b) -> c;` is something else...
>
> I'm not sure how that affects this? Using `decltype(auto)` as in 
> `decltype(auto) NAME() -> int { return 42; }` will recognize the arrow as a 
> trailing return type, but it doesn't really matter as its invalid syntax. I 
> tried to find valid syntax where the arrow would be seen as a trailing return 
> type, and I did with that second example

I'd just add a test to confirm that it works as expected.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Emilia Dreamer via Phabricator via cfe-commits]

rymiel added a comment.

In D141811#4068180 , 
@HazardyKnusperkeks wrote:

> I thought about `auto NAME() -> int { return 42; }`.
>
> `decltype(auto) a = (b) -> c;` is something else...

I'm not sure how that affects this? Using `decltype(auto)` as in 
`decltype(auto) NAME() -> int { return 42; }` will recognize the arrow as a 
trailing return type, but it doesn't really matter as its invalid syntax. I 
tried to find valid syntax where the arrow would be seen as a trailing return 
type, and I did with that second example


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D141811#4064428 , @rymiel wrote:

> In D141811#4055485 , 
> @HazardyKnusperkeks wrote:
>
>> What about `decltype(auto)`?
>
> Turns out this is a problem even without this patch:
>
> `decltype(auto) a = (b) -> c;`
>
> I'm sure this could somehow be valid c++ syntax, but it does have to be at 
> the top scope. I suppose I could fix it in this patch.

I thought about `auto NAME() -> int { return 42; }`.

`decltype(auto) a = (b) -> c;` is something else...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Emilia Dreamer via Phabricator via cfe-commits]

rymiel added a comment.

I suppose this means `auto a = (b) -> c;` is also technically broken, actually, 
and there's no easy fix for that.

I did have an idea for adding an additional pass taking place somewhere in 
`TokenAnnotator::calculateFormattingInformation`, which would detect trailing 
return types using a bit more context, as a potential fix for 
https://github.com/llvm/llvm-project/issues/41495. I suppose I could make that 
theoretically replace all of this AutoFound logic. I should actually try to see 
if my idea would work at all though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Emilia Dreamer via Phabricator via cfe-commits]

rymiel planned changes to this revision.
rymiel added a comment.

In D141811#4055485 , 
@HazardyKnusperkeks wrote:

> What about `decltype(auto)`?

Turns out this is a problem even without this patch:

`decltype(auto) a = (b) -> c;`

I'm sure this could somehow be valid c++ syntax, but it does have to be at the 
top scope. I suppose I could fix it in this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

What about `decltype(auto)`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141811/new/

https://reviews.llvm.org/D141811

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141811: [clang-format] Allow trailing return types in macros

[Emilia Dreamer via Phabricator via cfe-commits]

rymiel created this revision.
rymiel added reviewers: HazardyKnusperkeks, owenpan, MyDeveloperDay.
rymiel added a project: clang-format.
Herald added a project: All.
rymiel requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The trailing return type arrow checker verifies that a declaration is
being parsed, however, this isn't true when inside of macros.

It turns out the existence of the auto keyword is enough to make
sure that we're dealing with a trailing return type, and whether we're
in a declaration doesn't matter.

Fixes https://github.com/llvm/llvm-project/issues/47664


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141811

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7986,6 +7986,10 @@
"auto aa(T t)\n"
"-> decltype(eaaa(t.a).());");
 
+  verifyFormat("#define MAKE_DEF(NAME) 
"
+   "\\\n"
+   "  auto NAME() -> int { return 42; }");
+
   // Not trailing return types.
   verifyFormat("void f() { auto a = b->c(); }");
   verifyFormat("auto a = p->foo();");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1880,7 +1880,8 @@
 } else if (Current.is(tok::arrow) &&
Style.Language == FormatStyle::LK_Java) {
   Current.setType(TT_LambdaArrow);
-} else if (Current.is(tok::arrow) && AutoFound && Line.MustBeDeclaration &&
+} else if (Current.is(tok::arrow) && AutoFound &&
+   (Line.MustBeDeclaration || Line.InPPDirective) &&
Current.NestingLevel == 0 &&
!Current.Previous->isOneOf(tok::kw_operator, tok::identifier)) {
   // not auto operator->() -> xxx;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7986,6 +7986,10 @@
"auto aa(T t)\n"
"-> decltype(eaaa(t.a).());");
 
+  verifyFormat("#define MAKE_DEF(NAME) "
+   "\\\n"
+   "  auto NAME() -> int { return 42; }");
+
   // Not trailing return types.
   verifyFormat("void f() { auto a = b->c(); }");
   verifyFormat("auto a = p->foo();");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1880,7 +1880,8 @@
 } else if (Current.is(tok::arrow) &&
Style.Language == FormatStyle::LK_Java) {
   Current.setType(TT_LambdaArrow);
-} else if (Current.is(tok::arrow) && AutoFound && Line.MustBeDeclaration &&
+} else if (Current.is(tok::arrow) && AutoFound &&
+   (Line.MustBeDeclaration || Line.InPPDirective) &&
Current.NestingLevel == 0 &&
!Current.Previous->isOneOf(tok::kw_operator, tok::identifier)) {
   // not auto operator->() -> xxx;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits