[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)
@@ -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)
@@ -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)
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)
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)
@@ -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)
@@ -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)
@@ -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)
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)
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)
@@ -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)
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)
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)
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)
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)
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)
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)
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)
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)
@@ -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)
@@ -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)
@@ -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)
@@ -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