Skip to content

[Format] Fix isStartOfName to recognize attributes #76804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 15, 2024
Merged

[Format] Fix isStartOfName to recognize attributes #76804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d8598a3
[Format] Fix isStartOfName to recognize attributes
ilya-biryukov Jan 3, 2024
6ba6f35
Update tests
ilya-biryukov Jan 3, 2024
be3b337
Update clang/lib/Format/TokenAnnotator.cpp
ilya-biryukov Jan 12, 2024
8060996
Remove the unused #include of gmock
ilya-biryukov Jan 12, 2024
18d06c2
Merge branch 'main' into format3
ilya-biryukov Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions clang/lib/Format/Format.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1701,8 +1701,6 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
/*BasedOnStyle=*/"google",
},
};
GoogleStyle.AttributeMacros.push_back("GUARDED_BY");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

GoogleStyle.AttributeMacros.push_back("ABSL_GUARDED_BY");

GoogleStyle.SpacesBeforeTrailingComments = 2;
GoogleStyle.Standard = FormatStyle::LS_Auto;
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Format/TokenAnnotator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2209,7 +2209,8 @@ class AnnotatingParser {
(!NextNonComment && !Line.InMacroBody) ||
(NextNonComment &&
(NextNonComment->isPointerOrReference() ||
NextNonComment->isOneOf(tok::identifier, tok::string_literal)))) {
NextNonComment->is(tok::string_literal) ||
(Line.InPragmaDirective && NextNonComment->is(tok::identifier))))) {
return false;
}

Expand Down
8 changes: 2 additions & 6 deletions clang/unittests/Format/FormatTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
//===----------------------------------------------------------------------===//

#include "FormatTestBase.h"
#include "gmock/gmock.h"

#define DEBUG_TYPE "format-test"

Expand Down Expand Up @@ -8563,9 +8562,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 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
Expand Down Expand Up @@ -10158,11 +10154,11 @@ TEST_F(FormatTest, ReturnTypeBreakingStyle) {
getGoogleStyleWithColumns(40));
verifyFormat("Tttttttttttttttttttttttt ppppppppppppppp\n"
" ABSL_GUARDED_BY(mutex1)\n"
" ABSL_GUARDED_BY(mutex2);",
" ABSL_GUARDED_BY(mutex2);",
getGoogleStyleWithColumns(40));
verifyFormat("Tttttt 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;");
Expand Down