[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-08 Thread Ilya Biryukov via cfe-commits 


@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");

ilya-biryukov wrote:

@HazardyKnusperkeks friendly ping. Any thoughts on including a few more 
attributes into the (the first list of 4 elements) vs landing this change and 
relying on implicit formatting of those as function names?

I am happy to choose one of the two options arbitrarily, but I don't have 
enough context on `clang-format` to understand which approach is preferable, so 
I would love to get an opinion from someone in the `clang-format` community.

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-08 Thread kadir çetinkaya via cfe-commits 


@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");

kadircet wrote:

FWIW, not recognizing `foo` in `int foo BAR` as start-of-name looks like a big 
enough regression (which seems to be the main reason behind the line-braking 
behavior change), independent of whatever we do with the list of 
attribute-macros, I believe we should still make sure annotations for `foo` are 
correct rather urgently. so I am actually still in favor of landing this patch 
as-is, rather than trying to fix final formatting in a bunch of special cases 
by updating `AtrributeMacros` list.

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-08 Thread Owen Pan via cfe-commits 

https://github.com/owenca edited https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-08 Thread Owen Pan via cfe-commits 

owenca wrote:

> > can you also add a test to clang/unittests/Format/TokenAnnotatorTest.cpp 
> > that ensures trailing attribute-like macros receive `StartOfName` 
> > annotation to make sure we don't regress the signal in the future?
> 
> ok, that opened a whole can of worms.
> 
> ```
>  Tokens = annotate("void foo GUARDED_BY(x)");
> ```
> 
> gets annotated as
> 
> ```
> {(void, "void" , Unknown),
>   (identifier, "foo" , StartOfName),
>   (identifier, "GUARDED_BY" , FunctionDeclarationName),
>   (l_paren, "(" , Unknown),
>   (identifier, "x" , Unknown),
>   (r_paren, ")" , Unknown),
>   (eof, "" , Unknown)}
> ```
> 
> I expected to get some heuristics for attributes, but instead `GUARDED_BY` 
> gets annotated as a function declaration name. It feels that the current 
> behavior is a result of two mistakes cancelling each other out. I don't think 
> adding a unit test like this is warranted, even if formatting behavior is 
> actually correct.
> 
> @owenca what are your thoughts on this change and whether we should add a 
> test here?

We usually add a FIXME test wrapped in a `#if 0` block:
```
// FIXME: ...
#if 0
Tokens = annotate("void foo GUARDED_BY(x);");
...
Tokens = annotate("void foo GUARDED_BY(x) {}");
...
#endif
```

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-08 Thread Owen Pan via cfe-commits 


@@ -2209,7 +2209,8 @@ class AnnotatingParser {
 (!NextNonComment && !Line.InMacroBody) ||
 (NextNonComment &&
  (NextNonComment->isPointerOrReference() ||
-  NextNonComment->isOneOf(tok::identifier, tok::string_literal {
+  (Line.InPragmaDirective &&
+   NextNonComment->isOneOf(tok::identifier, tok::string_literal) {

owenca wrote:

```suggestion
  NextNonComment->is(tok::string_literal) ||
  (Line.InPragmaDirective && NextNonComment->is(tok::identifier) {
```

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-08 Thread Owen Pan via cfe-commits 


@@ -8498,9 +8498,6 @@ TEST_F(FormatTest, 
BreaksFunctionDeclarationsWithTrailingTokens) {
"__attribute__((unused));");
 
   Style = getGoogleStyle();
-  ASSERT_THAT(Style.AttributeMacros,

owenca wrote:

Please also delete line 10:
```
#include "gmock/gmock.h"
```

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-09 Thread Björn Schäpers via cfe-commits 


@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");

HazardyKnusperkeks wrote:

I'm open in all directions.

When clang-format does format attribute macros out of the box correctly, that 
is nice. But I wouldn't put (too much) work into it, if declaring them to 
clang-format as what they are fixes all misformatting.

Thus I would keep the entries in `AttributeMacros`.

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-12 Thread Ilya Biryukov via cfe-commits 

https://github.com/ilya-biryukov updated 
https://github.com/llvm/llvm-project/pull/76804

>From d8598a382fb1496a96d6ff8317c06cf73606ad84 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov 
Date: Wed, 3 Jan 2024 11:07:17 +0100
Subject: [PATCH 1/3] [Format] Fix isStartOfName to recognize attributes

This addresses a problem with formatting attributes in a different way
as before. For some context:
- 199fc973ced20016b04ba540cf63a1d4914fa513 changed `isStartOfName` to
  fix problems inside macro directives (judging by the added tests), but
  this behavior changed formatting of attribute macros in an undesirable
  way.
- efeb546865c233dfa7706ee0316c676de9f69897 changed Google format style
  to fix some widely used attributes.

Instead of changing the format style, this commit specializes behavior
introduced in 199fc973ced20016b04ba540cf63a1d4914fa513 to macro
directives. This seems to work well in both cases.

Also update the test with two `GUARDED_BY` directives. While the
formatting after efeb546865c233dfa7706ee0316c676de9f69897 seems better,
this case is rare enough to not warrant the extra complexity. We are
reverting it back to the state it had before
efeb546865c233dfa7706ee0316c676de9f69897.
---
 clang/lib/Format/Format.cpp | 2 --
 clang/lib/Format/TokenAnnotator.cpp | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index f798d555bf9929..38974f578fe1d2 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");
-  GoogleStyle.AttributeMacros.push_back("ABSL_GUARDED_BY");
 
   GoogleStyle.SpacesBeforeTrailingComments = 2;
   GoogleStyle.Standard = FormatStyle::LS_Auto;
diff --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index 3ac3aa3c5e3a22..94fe5b21cfc6e6 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2209,7 +2209,8 @@ class AnnotatingParser {
 (!NextNonComment && !Line.InMacroBody) ||
 (NextNonComment &&
  (NextNonComment->isPointerOrReference() ||
-  NextNonComment->isOneOf(tok::identifier, tok::string_literal {
+  (Line.InPragmaDirective &&
+   NextNonComment->isOneOf(tok::identifier, tok::string_literal) {
   return false;
 }
 

>From 6ba6f35e97af2299bbd5364fd535f31962a7eb4c Mon Sep 17 00:00:00 2001
From: Ilya Biryukov 
Date: Wed, 3 Jan 2024 11:22:26 +0100
Subject: [PATCH 2/3] Update tests

---
 clang/unittests/Format/FormatTest.cpp | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..fbbeddc29cfb5f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -8498,9 +8498,6 @@ TEST_F(FormatTest, 
BreaksFunctionDeclarationsWithTrailingTokens) {
"__attribute__((unused));");
 
   Style = getGoogleStyle();
-  ASSERT_THAT(Style.AttributeMacros,
-  testing::AllOf(testing::Contains("GUARDED_BY"),
- testing::Contains("ABSL_GUARDED_BY")));
 
   verifyFormat(
   "bool 
a\n"
@@ -10093,11 +10090,11 @@ TEST_F(FormatTest, ReturnTypeBreakingStyle) {
getGoogleStyleWithColumns(40));
   verifyFormat("Tttt ppp\n"
"ABSL_GUARDED_BY(mutex1)\n"
-   "ABSL_GUARDED_BY(mutex2);",
+   "ABSL_GUARDED_BY(mutex2);",
getGoogleStyleWithColumns(40));
   verifyFormat("Tt f(int a, int b)\n"
"ABSL_GUARDED_BY(mutex1)\n"
-   "ABSL_GUARDED_BY(mutex2);",
+   "ABSL_GUARDED_BY(mutex2);",
getGoogleStyleWithColumns(40));
   // * typedefs
   verifyGoogleFormat("typedef ATTR(X) char x;");

>From be3b337199fdc71718872e56aed19d89fa4447a1 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov 
Date: Fri, 12 Jan 2024 15:28:56 +0100
Subject: [PATCH 3/3] Update clang/lib/Format/TokenAnnotator.cpp

Co-authored-by: Owen Pan 
---
 clang/lib/Format/TokenAnnotator.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index 94fe5b21cfc6e6..7d030db45243a9 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2209,8 +2209,8 @@ class AnnotatingParser {
 (!NextNonComment && !Line.InMacroBody) ||
 (NextNonComment &&
  (NextNonComment->isPointerOrReference() ||
-  (Line.InPragmaDirective &&
-   NextNonComment->isOneOf(tok::identifier, tok::string_literal) {
+  NextNonComment->i

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-12 Thread Ilya Biryukov via cfe-commits 

https://github.com/ilya-biryukov updated 
https://github.com/llvm/llvm-project/pull/76804

>From d8598a382fb1496a96d6ff8317c06cf73606ad84 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov 
Date: Wed, 3 Jan 2024 11:07:17 +0100
Subject: [PATCH 1/4] [Format] Fix isStartOfName to recognize attributes

This addresses a problem with formatting attributes in a different way
as before. For some context:
- 199fc973ced20016b04ba540cf63a1d4914fa513 changed `isStartOfName` to
  fix problems inside macro directives (judging by the added tests), but
  this behavior changed formatting of attribute macros in an undesirable
  way.
- efeb546865c233dfa7706ee0316c676de9f69897 changed Google format style
  to fix some widely used attributes.

Instead of changing the format style, this commit specializes behavior
introduced in 199fc973ced20016b04ba540cf63a1d4914fa513 to macro
directives. This seems to work well in both cases.

Also update the test with two `GUARDED_BY` directives. While the
formatting after efeb546865c233dfa7706ee0316c676de9f69897 seems better,
this case is rare enough to not warrant the extra complexity. We are
reverting it back to the state it had before
efeb546865c233dfa7706ee0316c676de9f69897.
---
 clang/lib/Format/Format.cpp | 2 --
 clang/lib/Format/TokenAnnotator.cpp | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index f798d555bf9929..38974f578fe1d2 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");
-  GoogleStyle.AttributeMacros.push_back("ABSL_GUARDED_BY");
 
   GoogleStyle.SpacesBeforeTrailingComments = 2;
   GoogleStyle.Standard = FormatStyle::LS_Auto;
diff --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index 3ac3aa3c5e3a22..94fe5b21cfc6e6 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2209,7 +2209,8 @@ class AnnotatingParser {
 (!NextNonComment && !Line.InMacroBody) ||
 (NextNonComment &&
  (NextNonComment->isPointerOrReference() ||
-  NextNonComment->isOneOf(tok::identifier, tok::string_literal {
+  (Line.InPragmaDirective &&
+   NextNonComment->isOneOf(tok::identifier, tok::string_literal) {
   return false;
 }
 

>From 6ba6f35e97af2299bbd5364fd535f31962a7eb4c Mon Sep 17 00:00:00 2001
From: Ilya Biryukov 
Date: Wed, 3 Jan 2024 11:22:26 +0100
Subject: [PATCH 2/4] Update tests

---
 clang/unittests/Format/FormatTest.cpp | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..fbbeddc29cfb5f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -8498,9 +8498,6 @@ TEST_F(FormatTest, 
BreaksFunctionDeclarationsWithTrailingTokens) {
"__attribute__((unused));");
 
   Style = getGoogleStyle();
-  ASSERT_THAT(Style.AttributeMacros,
-  testing::AllOf(testing::Contains("GUARDED_BY"),
- testing::Contains("ABSL_GUARDED_BY")));
 
   verifyFormat(
   "bool 
a\n"
@@ -10093,11 +10090,11 @@ TEST_F(FormatTest, ReturnTypeBreakingStyle) {
getGoogleStyleWithColumns(40));
   verifyFormat("Tttt ppp\n"
"ABSL_GUARDED_BY(mutex1)\n"
-   "ABSL_GUARDED_BY(mutex2);",
+   "ABSL_GUARDED_BY(mutex2);",
getGoogleStyleWithColumns(40));
   verifyFormat("Tt f(int a, int b)\n"
"ABSL_GUARDED_BY(mutex1)\n"
-   "ABSL_GUARDED_BY(mutex2);",
+   "ABSL_GUARDED_BY(mutex2);",
getGoogleStyleWithColumns(40));
   // * typedefs
   verifyGoogleFormat("typedef ATTR(X) char x;");

>From be3b337199fdc71718872e56aed19d89fa4447a1 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov 
Date: Fri, 12 Jan 2024 15:28:56 +0100
Subject: [PATCH 3/4] Update clang/lib/Format/TokenAnnotator.cpp

Co-authored-by: Owen Pan 
---
 clang/lib/Format/TokenAnnotator.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index 94fe5b21cfc6e6..7d030db45243a9 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2209,8 +2209,8 @@ class AnnotatingParser {
 (!NextNonComment && !Line.InMacroBody) ||
 (NextNonComment &&
  (NextNonComment->isPointerOrReference() ||
-  (Line.InPragmaDirective &&
-   NextNonComment->isOneOf(tok::identifier, tok::string_literal) {
+  NextNonComment->i

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-12 Thread Ilya Biryukov via cfe-commits 


@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");

ilya-biryukov wrote:

I think we should postpone the inclusion of those names into `AtrributeMacros`  
and land the patch as is.

It seems that there is agreement among everyone that having this formatting 
without explicit `AtrributeMacros` is desirable, so landing as is is a 
no-brainer.

Whether we should add common macro names into `AttributeMacros` is more 
contentious, so I think we may need more data to back up our decision to go 
either way. I have some examples where not having those names in the config 
leads to undesirable formatting, but I would share them in a follow-up 
conversation.

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-12 Thread Ilya Biryukov via cfe-commits 

ilya-biryukov wrote:

@owenca, @HazardyKnusperkeks could you please take another look and approve if 
this looks good to you?
I believe all comments should be addressed at this point.

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-12 Thread Owen Pan via cfe-commits 

https://github.com/owenca approved this pull request.


https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-12 Thread Björn Schäpers via cfe-commits 

https://github.com/HazardyKnusperkeks approved this pull request.


https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-03 Thread Ilya Biryukov via cfe-commits 

https://github.com/ilya-biryukov created 
https://github.com/llvm/llvm-project/pull/76804

This addresses a problem with formatting attributes. Some context:
- 199fc973ced20016b04ba540cf63a1d4914fa513 changed `isStartOfName` to fix 
problems inside macro directives (judging by the added tests), but this 
behavior changed formatting of attribute macros in an undesirable way.
- efeb546865c233dfa7706ee0316c676de9f69897 changed Google format style to fix 
some widely used attributes.

Instead of changing the format style, this commit specializes behavior 
introduced in 199fc973ced20016b04ba540cf63a1d4914fa513 to macro directives. 
This seems to work well in both cases.

Also update the test with two `GUARDED_BY` directives. While the formatting 
after efeb546865c233dfa7706ee0316c676de9f69897 seems better, this case is rare 
enough to not warrant the extra complexity. We are reverting it back to the 
state it had before
efeb546865c233dfa7706ee0316c676de9f69897.

>From d8598a382fb1496a96d6ff8317c06cf73606ad84 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov 
Date: Wed, 3 Jan 2024 11:07:17 +0100
Subject: [PATCH] [Format] Fix isStartOfName to recognize attributes

This addresses a problem with formatting attributes in a different way
as before. For some context:
- 199fc973ced20016b04ba540cf63a1d4914fa513 changed `isStartOfName` to
  fix problems inside macro directives (judging by the added tests), but
  this behavior changed formatting of attribute macros in an undesirable
  way.
- efeb546865c233dfa7706ee0316c676de9f69897 changed Google format style
  to fix some widely used attributes.

Instead of changing the format style, this commit specializes behavior
introduced in 199fc973ced20016b04ba540cf63a1d4914fa513 to macro
directives. This seems to work well in both cases.

Also update the test with two `GUARDED_BY` directives. While the
formatting after efeb546865c233dfa7706ee0316c676de9f69897 seems better,
this case is rare enough to not warrant the extra complexity. We are
reverting it back to the state it had before
efeb546865c233dfa7706ee0316c676de9f69897.
---
 clang/lib/Format/Format.cpp | 2 --
 clang/lib/Format/TokenAnnotator.cpp | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index f798d555bf9929..38974f578fe1d2 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");
-  GoogleStyle.AttributeMacros.push_back("ABSL_GUARDED_BY");
 
   GoogleStyle.SpacesBeforeTrailingComments = 2;
   GoogleStyle.Standard = FormatStyle::LS_Auto;
diff --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index 3ac3aa3c5e3a22..94fe5b21cfc6e6 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2209,7 +2209,8 @@ class AnnotatingParser {
 (!NextNonComment && !Line.InMacroBody) ||
 (NextNonComment &&
  (NextNonComment->isPointerOrReference() ||
-  NextNonComment->isOneOf(tok::identifier, tok::string_literal {
+  (Line.InPragmaDirective &&
+   NextNonComment->isOneOf(tok::identifier, tok::string_literal) {
   return false;
 }
 

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

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-03 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang-format

Author: Ilya Biryukov (ilya-biryukov)


Changes

This addresses a problem with formatting attributes. Some context:
- 199fc973ced20016b04ba540cf63a1d4914fa513 changed `isStartOfName` to fix 
problems inside macro directives (judging by the added tests), but this 
behavior changed formatting of attribute macros in an undesirable way.
- efeb546865c233dfa7706ee0316c676de9f69897 changed Google format style to fix 
some widely used attributes.

Instead of changing the format style, this commit specializes behavior 
introduced in 199fc973ced20016b04ba540cf63a1d4914fa513 to macro directives. 
This seems to work well in both cases.

Also update the test with two `GUARDED_BY` directives. While the formatting 
after efeb546865c233dfa7706ee0316c676de9f69897 seems better, this case is rare 
enough to not warrant the extra complexity. We are reverting it back to the 
state it had before
efeb546865c233dfa7706ee0316c676de9f69897.

---
Full diff: https://github.com/llvm/llvm-project/pull/76804.diff


2 Files Affected:

- (modified) clang/lib/Format/Format.cpp (-2) 
- (modified) clang/lib/Format/TokenAnnotator.cpp (+2-1) 


``diff
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index f798d555bf9929..38974f578fe1d2 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");
-  GoogleStyle.AttributeMacros.push_back("ABSL_GUARDED_BY");
 
   GoogleStyle.SpacesBeforeTrailingComments = 2;
   GoogleStyle.Standard = FormatStyle::LS_Auto;
diff --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index 3ac3aa3c5e3a22..94fe5b21cfc6e6 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2209,7 +2209,8 @@ class AnnotatingParser {
 (!NextNonComment && !Line.InMacroBody) ||
 (NextNonComment &&
  (NextNonComment->isPointerOrReference() ||
-  NextNonComment->isOneOf(tok::identifier, tok::string_literal {
+  (Line.InPragmaDirective &&
+   NextNonComment->isOneOf(tok::identifier, tok::string_literal) {
   return false;
 }
 

``




https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-03 Thread Ilya Biryukov via cfe-commits 

https://github.com/ilya-biryukov updated 
https://github.com/llvm/llvm-project/pull/76804

>From d8598a382fb1496a96d6ff8317c06cf73606ad84 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov 
Date: Wed, 3 Jan 2024 11:07:17 +0100
Subject: [PATCH 1/2] [Format] Fix isStartOfName to recognize attributes

This addresses a problem with formatting attributes in a different way
as before. For some context:
- 199fc973ced20016b04ba540cf63a1d4914fa513 changed `isStartOfName` to
  fix problems inside macro directives (judging by the added tests), but
  this behavior changed formatting of attribute macros in an undesirable
  way.
- efeb546865c233dfa7706ee0316c676de9f69897 changed Google format style
  to fix some widely used attributes.

Instead of changing the format style, this commit specializes behavior
introduced in 199fc973ced20016b04ba540cf63a1d4914fa513 to macro
directives. This seems to work well in both cases.

Also update the test with two `GUARDED_BY` directives. While the
formatting after efeb546865c233dfa7706ee0316c676de9f69897 seems better,
this case is rare enough to not warrant the extra complexity. We are
reverting it back to the state it had before
efeb546865c233dfa7706ee0316c676de9f69897.
---
 clang/lib/Format/Format.cpp | 2 --
 clang/lib/Format/TokenAnnotator.cpp | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index f798d555bf9929..38974f578fe1d2 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");
-  GoogleStyle.AttributeMacros.push_back("ABSL_GUARDED_BY");
 
   GoogleStyle.SpacesBeforeTrailingComments = 2;
   GoogleStyle.Standard = FormatStyle::LS_Auto;
diff --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index 3ac3aa3c5e3a22..94fe5b21cfc6e6 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2209,7 +2209,8 @@ class AnnotatingParser {
 (!NextNonComment && !Line.InMacroBody) ||
 (NextNonComment &&
  (NextNonComment->isPointerOrReference() ||
-  NextNonComment->isOneOf(tok::identifier, tok::string_literal {
+  (Line.InPragmaDirective &&
+   NextNonComment->isOneOf(tok::identifier, tok::string_literal) {
   return false;
 }
 

>From 6ba6f35e97af2299bbd5364fd535f31962a7eb4c Mon Sep 17 00:00:00 2001
From: Ilya Biryukov 
Date: Wed, 3 Jan 2024 11:22:26 +0100
Subject: [PATCH 2/2] Update tests

---
 clang/unittests/Format/FormatTest.cpp | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..fbbeddc29cfb5f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -8498,9 +8498,6 @@ TEST_F(FormatTest, 
BreaksFunctionDeclarationsWithTrailingTokens) {
"__attribute__((unused));");
 
   Style = getGoogleStyle();
-  ASSERT_THAT(Style.AttributeMacros,
-  testing::AllOf(testing::Contains("GUARDED_BY"),
- testing::Contains("ABSL_GUARDED_BY")));
 
   verifyFormat(
   "bool 
a\n"
@@ -10093,11 +10090,11 @@ TEST_F(FormatTest, ReturnTypeBreakingStyle) {
getGoogleStyleWithColumns(40));
   verifyFormat("Tttt ppp\n"
"ABSL_GUARDED_BY(mutex1)\n"
-   "ABSL_GUARDED_BY(mutex2);",
+   "ABSL_GUARDED_BY(mutex2);",
getGoogleStyleWithColumns(40));
   verifyFormat("Tt f(int a, int b)\n"
"ABSL_GUARDED_BY(mutex1)\n"
-   "ABSL_GUARDED_BY(mutex2);",
+   "ABSL_GUARDED_BY(mutex2);",
getGoogleStyleWithColumns(40));
   // * typedefs
   verifyGoogleFormat("typedef ATTR(X) char x;");

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

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-03 Thread kadir çetinkaya via cfe-commits 

https://github.com/kadircet approved this pull request.

can you also add a test to clang/unittests/Format/TokenAnnotatorTest.cpp that 
ensures trailing attribute-like macros receive `StartOfName` annotation to make 
sure we don't regress the signal in the future?

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-03 Thread Ilya Biryukov via cfe-commits 

ilya-biryukov wrote:

> can you also add a test to clang/unittests/Format/TokenAnnotatorTest.cpp that 
> ensures trailing attribute-like macros receive `StartOfName` annotation to 
> make sure we don't regress the signal in the future?

ok, that opened a whole can of worms.
```
 Tokens = annotate("void foo GUARDED_BY(x)");
```
gets annotated as
```
{(void, "void" , Unknown),
  (identifier, "foo" , StartOfName),
  (identifier, "GUARDED_BY" , FunctionDeclarationName),
  (l_paren, "(" , Unknown),
  (identifier, "x" , Unknown),
  (r_paren, ")" , Unknown),
  (eof, "" , Unknown)}
```


I expected to get some heuristics for attributes, but instead `GUARDED_BY` gets 
annotated as a function declaration name.\
It feels that the current behavior is a result of two mistakes cancelling each 
other out. I don't think adding a unit test like this is warranted, even if 
formatting behavior is actually correct.

@owenca what are your thoughts on this change and whether we should add a test 
here?

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-03 Thread Björn Schäpers via cfe-commits 


@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");

HazardyKnusperkeks wrote:

Even if that would not be needed anymore to achieve the expected formatting, 
these are AttributeMacros, and should be declared as such.

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-03 Thread Björn Schäpers via cfe-commits 


@@ -10093,11 +10090,11 @@ TEST_F(FormatTest, ReturnTypeBreakingStyle) {
getGoogleStyleWithColumns(40));
   verifyFormat("Tttt ppp\n"
"ABSL_GUARDED_BY(mutex1)\n"
-   "ABSL_GUARDED_BY(mutex2);",
+   "ABSL_GUARDED_BY(mutex2);",

HazardyKnusperkeks wrote:

This looks wrong.

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-04 Thread Ilya Biryukov via cfe-commits 


@@ -10093,11 +10090,11 @@ TEST_F(FormatTest, ReturnTypeBreakingStyle) {
getGoogleStyleWithColumns(40));
   verifyFormat("Tttt ppp\n"
"ABSL_GUARDED_BY(mutex1)\n"
-   "ABSL_GUARDED_BY(mutex2);",
+   "ABSL_GUARDED_BY(mutex2);",

ilya-biryukov wrote:

It is wrong, but it was like the recent patch where I changed Google style. I 
mentioned it in the commit description  (quote follows):

> Also update the test with two GUARDED_BY directives. While the formatting 
> after 
> https://github.com/llvm/llvm-project/commit/efeb546865c233dfa7706ee0316c676de9f69897
>  seems better, this case is rare enough to not warrant the extra complexity. 
> We are reverting it back to the state it had before
https://github.com/llvm/llvm-project/commit/efeb546865c233dfa7706ee0316c676de9f69897.

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)

2024-01-04 Thread Ilya Biryukov via cfe-commits 


@@ -1698,8 +1698,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind 
Language) {
   /*BasedOnStyle=*/"google",
   },
   };
-  GoogleStyle.AttributeMacros.push_back("GUARDED_BY");

ilya-biryukov wrote:

They are attribute macros indeed, the problem is that we actually need more. 
The ones used with fields are at least:
- ABSL_PT_GUARDED_BY
- ABSL_ACQUIRED_AFTER
- ABSL_ACQUIRED_BEFORE
- ABSL_GUARDED_BY_FIXME

We could also consider including the annotations for functions, but the patch 
only broke formatting for variables, so it's not strictly necessary to unblock 
the release.

If we want to also include the ones that are used with functions (they are not 
strictly necessary because `clang-format` does a decent job there without 
config), we would need to add at least these:
- ABSL_EXCLUSIVE_LOCKS_REQUIRED
- ABSL_LOCKS_EXCLUDED
- ABSL_LOCK_RETURNED
- ABSL_EXCLUSIVE_LOCK_FUNCTION
- ABSL_EXCLUSIVE_TRYLOCK_FUNCTION
- ABSL_SHARED_TRYLOCK_FUNCTION
- ABSL_ASSERT_EXCLUSIVE_LOCK
- ABSL_ASSERT_SHARED_LOCK
- ABSL_NO_THREAD_SAFETY_ANALYSIS


I am not sure how to best approach it and would appreciate some guidance here. 
Should we have all these attribute macros inside `AttributeMacros` or should we 
aim for clang-format formatting them reasonably without configuration?

https://github.com/llvm/llvm-project/pull/76804
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits