[clang-format] Handle C variables with name that matches c++ access specifier

[psigillito]

Sorry, this is my first time using Phabricator. I am not sure if it matters that .arclint is changed in this review, it is not part of my commit.

This issue only appears to impact the accessSpecifier keywords. For example, a c variable declared with the name 'private' will be treated as an access specifier.

I tried to make parsing the identifiers more restrictive by requiring either an @ prefix for obj-c or a colon after the keyword for cpp. Unfortunately, that does not allow an incorrect access specifier (i.e without a colon) to be identified as an accessor. I think a missing colon is probably a common typo so I dont want to remove identifying a access specifier with a missing colon correctly.

I think the correct approach is to check if the var named 'private' is followed by an operator or symbol indicating it is variable.

[curdeius]

Does it fix https://github.com/llvm/llvm-project/issues/46915?

[MyDeveloperDay]

.arclint

5 ↗

(On Diff #400340)

remove this change

clang/lib/Format/UnwrappedLineFormatter.cpp

102–103

Style.isCpp()

clang/lib/Format/UnwrappedLineParser.cpp

2728

elide {

clang/unittests/Format/FormatTest.cpp

3127

This isn't enough of a test, if you are going to add this CppOperatorsFollowingVar then you'd better test every case

[curdeius]

Thanks for having a try on this.
However, I don't like this approach too much. You add many changes and a single test. That's not sufficient.
Also, handling C++ keywords in all cases (e.g. delete as a function name) *may* need to distinguish whether we format a C file or a C++ file. It's probably impossible to do this without user input (.h extension is used in both languages for example).
We'd maybe need to add C as language option and let the user specify the language (-x c?).
That in turn may be painful (because not automatic).
But, you may have a better solution.
My 2 cents.

.arclint

5 ↗

(On Diff #400340)

Unrelated. Please revert.

clang/lib/Format/UnwrappedLineFormatter.cpp

102–103

Please refactor to e.g. a lambda to ease the understanding. And use a series of ifs and returns.

[psigillito]

In D117416#3246838, @curdeius wrote:

Thanks for having a try on this.
However, I don't like this approach too much. You add many changes and a single test. That's not sufficient.
Also, handling C++ keywords in all cases (e.g. delete as a function name) *may* need to distinguish whether we format a C file or a C++ file. It's probably impossible to do this without user input (.h extension is used in both languages for example).
We'd maybe need to add C as language option and let the user specify the language (-x c?).
That in turn may be painful (because not automatic).
But, you may have a better solution.
My 2 cents.

Thanks, I am just starting to understand some of the code base so my changes should be taken with a grain of salt. The more I think about this, I do not think my change is correct. As you pointed out with the 'delete' keyword, my changes would not correctly handle a struct named 'delete' i.e.

struct delete foo = {0};
delete.val;

I think adding a cmd line argument specifying you are using C code is more correct. If the user does not specify they are using C, clang-format would format the code as it does now.

The only thing I don't like about this is that it might be kind of clumsy to only be able to specify your language is C. I can take a stab at implementing this if this is the route we want to go.

[HazardyKnusperkeks]

In D117416#3246838, @curdeius wrote:

Thanks for having a try on this.
However, I don't like this approach too much. You add many changes and a single test. That's not sufficient.
Also, handling C++ keywords in all cases (e.g. delete as a function name) *may* need to distinguish whether we format a C file or a C++ file. It's probably impossible to do this without user input (.h extension is used in both languages for example).
We'd maybe need to add C as language option and let the user specify the language (-x c?).
That in turn may be painful (because not automatic).
But, you may have a better solution.
My 2 cents.

I second just adding a new language.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

2722–2731

I get what your trying to do but what happens in this scenario

private::mynamespace::g_value -1;

[psigillito]

Removed old approach and started initial changes to add C language

[psigillito]

revert changes

[psigillito]

Hopefully this revision works

[psigillito]

Include multiple commits in review

[psigillito]

• undo delete

[psigillito]

• annoying arc changes

[psigillito]

These are just initial changes, there is still a lot of work and test cases to write. I figured I would put this out there to see if this is the direction we were thinking.

clang/lib/Format/UnwrappedLineParser.cpp

2722–2731

Yea, it doesnt parse correctly and is split to a new line after private:. I am going to revert all the changes in this code review and start looking at implementing C as its own language.

[MyDeveloperDay]

I'm not a fan of this approach of adding a "C" language, mainly because of the .h problem so ultimately it doesn't solve your problem.

I think this is overkill for what is basically the subtle handling of "public/private/protected/try/delete/new" being used as variables (which frankly is at least in my view not a bright idea in the first place!)

I think ultimately we can try to catch those corner cases but IMHO it doesn't need the addition of a whole new language for that, (we are not at that much on an impasse)

This review contains .arclint that needs removing, this review contains no tests

[MyDeveloperDay]

I actually thought you were on the right lines with your change to parseAccessSpecifier() surely if you see "private:" "public:" or "protected:" but not public:: that's when you call parseAccessSpecifier()

I think mostly I believe clang-format is a 96% done deal in terms of code, a lot of what we do here in my view is about closing those final "corner cases", sometimes our fixes are quite specific,

Something in the back of my head says this is a sledge hammer to crack a nut but that is just my view.. I'm just a little uncomfortable here with this change although it may open doors for other C specific changes, I'm not convinced this solves the problem I just think it moves it a little and maybe adds to complexity for what is likely a very small number of cases.

clang/lib/Format/TokenAnnotator.cpp

3238 ↗

(On Diff #400718)

I wonder how many people using C do this?

#define new(x) malloc(x)
#define delete(x) free(x)

Some at least

https://github.com/jbf/tonsai-scheme/blob/83d2a022f60fc17f9913c017fbde29e19cdf5b2b/src/memory.h

clang/lib/Format/UnwrappedLineFormatter.cpp

100

isC() these large OR/AND conditions become unreadable

clang/lib/Format/UnwrappedLineParser.cpp

1295

isC()

1678

I feel everywhere you put isCpp you'll end up doing isCpp() || is C()

This goes back to the arguments for a review I tried before D80079: [clang-format] [NFC] isCpp() is inconsistently used to mean both C++ and Objective C, add language specific isXXX() functions its really isCStyle() if we WERE going to pursue this, I'd want that and not have clauses added everywhere

clang/tools/clang-format/ClangFormat.cpp

124 ↗

(On Diff #400718)

Is this required for this patch or is this a nice to have, can we have a separate review for this, it may have merit in its own right but it needs it own tests

467 ↗

(On Diff #400718)

I don't understand what you are doing here or why? is this just for C it feels wrong

469 ↗

(On Diff #400718)

we don't use braces on single line if

wouldn't you use a tolower or toupper here?

[MyDeveloperDay]

For me the push back on adding the "C" language and having to pass it on the command line for the .h case is about VS Code, Vim, Visual Studio, Emacs having to recognize what language they are in and having to pass the -language=XXX flag, now we are pushing the interpretation of what language we are to them and not to us, which mean we are now out of control.

Each editor will make different assessments as to what language it is (likely only 98% correctly) and we'll get the blame when they say its C# and not C because they made the wrong decision

Personally, If we are going to add C as a language we need something that can control it without external intervention

something like..

// clang-format language='C'

or something that reads the header (like we used to have in Makefile)

---*- C++ -*-===//

especially if there is a sudo standard for that, and uses that as a language hint to say this is a "C" .h header

But I think it might be a mistake for us to rely on the editors to provide us that, and even adding the option could open a tsunami of incorrect language issues which we cannot solve.

[MyDeveloperDay]

Isn't the reality that developers using delete/new/try/catch/interface/public/protected/private/async/var/let as variables the real problem? I'm slightly averse to pandering to them, and I definitely don't want to make my world more complex just to accommodate their poor choice of variable name.

I know if someone used any of those words as variables in LLVM surely we'd tell them to change it. Honestly how much of a problem are we really talking about here that can't be handled by a few corner cases.

[curdeius]

As MyDeveloperDay justly pointed out, adding a new language enum LK_C doesn't seem to be the way to go, as you mostly duplicate all the conditions (probably missing some of them).
For the usability, it would be really painful to specify the language for each file that should be considered as C.
So, sorry for this forth and back, but now at least we see that your initial approach was actually better (but please add tests!).

We'd maybe need to add C as language option and let the user specify the language (-x c?).
That in turn may be painful (because not automatic).

Yeah, I should have been more explicit here.

.arclint

15–16 ↗

(On Diff #400718)

Please don't change unrelated files.

[MyDeveloperDay]

.arclint

15–16 ↗

(On Diff #400718)

I don't know about others but I personally don't use arcanist...

I stage my files, make a patch and upload manually

git add <files being modified>  (or `git ls files -m | xargs git add` )
git clang-format 
git diff --cached -U4 > patch_to_submitt.diff

I use the Create Diff/Update Diff functionality via the web interface (I feel that gives me more control)

Just my 2c

[curdeius]

.arclint

15–16 ↗

(On Diff #400718)

I do, but I never had to modify .arclint or whatever config file arcanist uses.
It just works as a charm.

Create and send a patch (if sending all changes not in main branch):

arc diff main --reviewers ...

Update a patch:

arc diff main --update --revision D123456789

[HazardyKnusperkeks]

In D117416#3250415, @MyDeveloperDay wrote:

I'm not a fan of this approach of adding a "C" language, mainly because of the .h problem so ultimately it doesn't solve your problem.

I think this is overkill for what is basically the subtle handling of "public/private/protected/try/delete/new" being used as variables (which frankly is at least in my view not a bright idea in the first place!)

I think ultimately we can try to catch those corner cases but IMHO it doesn't need the addition of a whole new language for that, (we are not at that much on an impasse)

This review contains .arclint that needs removing, this review contains no tests

I was actually thinking that adding C as language is the right way, and still am. We have stuff only for arcane C variants, why not push these in the language C and keep C++ free from it?

But I'm fine with either solution.

clang/lib/Format/UnwrappedLineFormatter.cpp

100

I'm more of a switch fan and not your isXXX(), but that's preference. ;)

clang/lib/Format/UnwrappedLineParser.cpp

1678

I think one could use isC(), isCpp(), isObjC(), and what is now isCpp(): isCish().

[psigillito]

Ok, as I was starting to add a new language, the scope of changes just continued to grow.

If you think it is worthwhile, I think I can fix this edge case for accessSpecifiers by cleaning up my old approach and adding some tests. I dont like having to add a big set of operators to check against for handling the case where there is a typo and the colon is missing i.e.

class foo { 
private
  bool jim;
public:
  bool bob;
};

I think this is probably the most common error so I think we should support it.

I don't think the delete issue in https://github.com/llvm/llvm-project/issues/46915 is worth the added complexity. Without specifying the language, it is too hard to interpret the programmer's intention. For example, these are totally valid as either a delete or a function call:

delete(foo) // foo is a pointer being deleted 
delete(bar) // bar is a parameter to a function

[MyDeveloperDay]

@HazardyKnusperkeks (its not without merits to add a "C" Language the more i think about it, but I'm not sure quite in the form presented here)

If we DID add a "C" language then we need to add something like you suggested isCpp() already means more than we think.

bool isCStyle() {
    return isCpp() || isC();
}

Personally I really don't want to see the below change or it will change the world, so it would be better if the change was just from !Style.isCpp() to !Style.isCStyle()

!Style.isCpp() && !Style.isC()

The ClangFormat.cpp changes are really unrelated and need to be removed (or resubmitted/discussed separately), and we really must have extensive unit tests if we are adding a new language (which this review doesn't have any) - (that means a new FormatCTests.cpp)

I'd personally want to see really limited use of isCStyle() and ONLY what's needed to fix the exact example/tests we'd add. (its all to easy to find/replace thinking you are doing the right thing when actually we may not, it needs to be targetted changes)

But bottom line, I just don't see how we fix the .h problem? other than if the .clang-format section doesn't have a Language: Cpp then we don't assume its C++ (problem is it already does)

(actually this is a request that users can limit the guessing of languages to only those languages present in the .clang-format file)

This would be good for "C" projects but not much use for C++ or C++/C projects.

It wouldn't solve the delete(foo); issue this can still be a function or a delete of a bracketed variable, or a macro... so I don't see it helps

And bottom line the Lexer will give it to you as tok::kw_private and not as tok::identifier regardless of your language and so you'll need to perhaps do something in FormatTokenLexer to transform it back to an identifier (which is possible) checkout tryMergeLessLess() etc...

[psigillito]

• revert to corner case handling with private

[psigillito]

• revert bad changes and arclint

[MyDeveloperDay]

new nits but basically I think this looks like it might handle this ok? LGTM

We need to run this across a large code base to check

clang/lib/Format/UnwrappedLineFormatter.cpp

106

remove extraneous {} braces

[HazardyKnusperkeks]

clang/lib/Format/FormatToken.h

126

And maybe choose a different container: https://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc

clang/lib/Format/UnwrappedLineFormatter.cpp

104

In clang-format all lambdas I've seen start with a capital letter.

107

Please just return. And then drop the else.

clang/lib/Format/UnwrappedLineParser.cpp

2710

[psigillito]

Review Changes

[psigillito]

undo arclint change

[psigillito]

Base diff on upstream

[psigillito]

• revert bad arclint changes

[psigillito]

• revert of .arclint file delete

[curdeius]

Last nits, apart from this clean up, I'm OK.

clang/lib/Format/FormatToken.h

126

Not done it seems.
Please rename and use a different type. Maybe ImmutableSet? Or just a sorted std::vector?

clang/lib/Format/UnwrappedLineFormatter.cpp

109

Nit here and elsewhere: full stop at the end of comments.

113

116

Please use contains instead of count if using ImmutableSet.

clang/lib/Format/UnwrappedLineParser.cpp

2710

2723–2729

2724

Ditto. Use contains.

[psigillito]

clang/lib/Format/FormatToken.h

126

ok updated to sorted std::vector

clang/lib/Format/UnwrappedLineFormatter.cpp

104

Updated.

[curdeius]

Have you updated the diff? I still see std::set and not a sorted vector.

[psigillito]

• Merge branch 'main' of https://github.com/llvm/llvm-project
• code review syntax cleanup, sorted vector

[psigillito]

• revert auto formatting comments

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

119–121

Drop Braces

122–126

Use std::binary_search instead of std::lower_bound. That should simplify the following if.

[curdeius]

clang/lib/Format/FormatToken.h

125

clang/lib/Format/UnwrappedLineFormatter.cpp

122–129

Please do something along these lines. The idea is to put a cheaper check first.

Note. I haven't tested nor formatted these lines.

905

Revert.

clang/lib/Format/UnwrappedLineParser.cpp

2716–2718

Please use binary_search and put it inside the else branch to avoid it if the first condition is satisfied.
Something like:

if (FormatTok->Tok.is(tok::colon)) {
...
} else if (!binary_search(...) {
} else if (...) {
}

Also, this code and the code in UnwrappedLineFormatter are pretty much similar.
Can we remove this duplication by e.g. setting the token kind here and checking it in the formatter?

[psigillito]

clang/lib/Format/UnwrappedLineParser.cpp

2716–2718

Yes, since we now set the kind to 'identifier' here, the binary_search check in the formatter is unnecessary. Updated the formatter to no longer do the check.

[psigillito]

• review changes, lower_bound to binary_search

[curdeius]

LGTM but please add an assert(is_sorted).

clang/lib/Format/FormatToken.h

126

Is there a place anywhere that you verify it's sorted?
If no, please add an assert.

[HazardyKnusperkeks]

+1 for assert

clang/lib/Format/FormatToken.h

126

Could initialize from a lambda in which the assert is located, thus it is only executed once.

static const std::vector<clang::tok::TokenKind> COperatorsFollowingVar = []{
  std::vector<clang::tok::TokenKind> Ret...;
  assert(...);
  return Ret;
}();

[psigillito]

• wrap vector in function

[psigillito]

• unnecessary whitespace

[HazardyKnusperkeks]

Have you commit access, or do you need some one to commit it for you?

[psigillito]

@HazardyKnusperkeks I do not have commit access. This is my first commit to the project. Do I just need to issue an 'arc land' command?

[HazardyKnusperkeks]

In D117416#3282775, @psigillito wrote:

@HazardyKnusperkeks I do not have commit access. This is my first commit to the project. Do I just need to issue an 'arc land' command?

I've never used arc, so I can't tell you anything about that.

You need to apply for commit access, that's no big deal, you can find the address in the LLVM documentation. Or you post name and email for the commit here and someone will commit it on your behalf.

[psigillito]

@HazardyKnusperkeks
Ok thanks, someone else can commit on my behalf while I wait for commit access.

name: Philip Sigillito
email: [e-mail]

[psigillito]

Do I need to care about these pre-build checks failing?

[HazardyKnusperkeks]

In D117416#3282853, @psigillito wrote:

Do I need to care about these pre-build checks failing?

The checks passed, the build on linux failed. But as far as I can see it's something in openmp. I assume you did run all the format tests, then everything should be fine.

[psigillito]

In D117416#3282937, @HazardyKnusperkeks wrote:

In D117416#3282853, @psigillito wrote:

Do I need to care about these pre-build checks failing?

The checks passed, the build on linux failed. But as far as I can see it's something in openmp. I assume you did run all the format tests, then everything should be fine.

Yes, I ran the format tests.

Thanks!

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(CSharpNamedArgumentColon)                                               \
   TYPE(CSharpNullable)                                                         \
   TYPE(CSharpNullConditionalLSquare)                                           \
   TYPE(CSharpGenericTypeConstraint)                                            \
   TYPE(CSharpGenericTypeConstraintColon)                                       \
   TYPE(CSharpGenericTypeConstraintComma)                                       \
   TYPE(Unknown)
 
+/// Sorted Operators that can follow a C variable.
+static const std::vector<clang::tok::TokenKind> COperatorsFollowingVar = {
+    tok::l_square,     tok::r_square,
+    tok::l_paren,      tok::r_paren,
+    tok::r_brace,      tok::period,
+    tok::ellipsis,     tok::ampamp,
+    tok::ampequal,     tok::star,
+    tok::starequal,    tok::plus,
+    tok::plusplus,     tok::plusequal,
+    tok::minus,        tok::arrow,
+    tok::minusminus,   tok::minusequal,
+    tok::exclaim,      tok::exclaimequal,
+    tok::slash,        tok::slashequal,
+    tok::percent,      tok::percentequal,
+    tok::less,         tok::lessless,
+    tok::lessequal,    tok::lesslessequal,
+    tok::greater,      tok::greatergreater,
+    tok::greaterequal, tok::greatergreaterequal,
+    tok::caret,        tok::caretequal,
+    tok::pipe,         tok::pipepipe,
+    tok::pipeequal,    tok::question,
+    tok::semi,         tok::equal,
+    tok::equalequal,   tok::comma};
+
 /// Determines the semantic type of a syntactic token, e.g. whether "<" is a
 /// template opener or binary operator.
 enum TokenType : uint8_t {
 #define TYPE(X) TT_##X,
   LIST_TOKEN_TYPES
 #undef TYPE
       NUM_TOKEN_TYPES
 };

clang/lib/Format/UnwrappedLineFormatter.cpp

public:
   }
 
 private:
   /// Get the offset of the line relatively to the level.
   ///
   /// For example, 'public:' labels in classes are offset by 1 or 2
   /// characters to the left from their level.
   int getIndentOffset(const FormatToken &RootToken) {
     if (Style.Language == FormatStyle::LK_Java || Style.isJavaScript() ||
         Style.isCSharp())
       return 0;
-    if (RootToken.isAccessSpecifier(false) ||
-        RootToken.isObjCAccessSpecifier() ||
-        (RootToken.isOneOf(Keywords.kw_signals, Keywords.kw_qsignals) &&
-         RootToken.Next && RootToken.Next->is(tok::colon))) {
+
+    auto IsAccessModifier = [this, &RootToken]() {
+      if (RootToken.isAccessSpecifier(Style.isCpp()))
+        return true;
+      else if (RootToken.isObjCAccessSpecifier())
+        return true;
+      // Handle Qt signals.
+      else if ((RootToken.isOneOf(Keywords.kw_signals, Keywords.kw_qsignals) &&
+                RootToken.Next && RootToken.Next->is(tok::colon)))
+        return true;
+      else if (RootToken.Next &&
+               RootToken.Next->isOneOf(Keywords.kw_slots, Keywords.kw_qslots) &&
+               RootToken.Next->Next && RootToken.Next->Next->is(tok::colon))
+        return true;
+      // Handle malformed access specifier e.g. 'private' without trailing ':'.
+      else if (RootToken.isAccessSpecifier(false)) {
+        if (!RootToken.Next) {
+          return true;
+        }
+        auto COperatorMatch = std::lower_bound(COperatorsFollowingVar.begin(),
+                                               COperatorsFollowingVar.end(),
+                                               RootToken.Next->Tok.getKind());
+        if ((COperatorMatch == COperatorsFollowingVar.end() ||
+             *COperatorMatch != RootToken.Next->Tok.getKind()) &&
+            !RootToken.Next->Tok.is(tok::coloncolon)) {
+          return true;
+        }
+      }
+      return false;
+    };
+
+    if (IsAccessModifier()) {
       // The AccessModifierOffset may be overridden by IndentAccessModifiers,
       // in which case we take a negative value of the IndentWidth to simulate
       // the upper indent level.
       return Style.IndentAccessModifiers ? -Style.IndentWidth
                                          : Style.AccessModifierOffset;
     }
     return 0;
   }
protected:
   /// The crucial idea here is that children always get formatted upon
   /// encountering the closing brace right after the nested block. Now, if we
   /// are currently trying to keep the "}" on the same line (i.e. \p NewLine is
   /// \c false), the entire block has to be kept on the same line (which is only
   /// possible if it fits on the line, only contains a single statement, etc.
   ///
   /// If \p NewLine is true, we format the nested block on separate lines, i.e.
   /// break after the "{", format all lines with correct indentation and the put
-  /// the closing "}" on yet another new line.
+  ///  the closing "}" on yet another new line.
   ///
   /// This enables us to keep the simple structure of the
   /// \c UnwrappedLineFormatter, where we only have two options for each token:
   /// break or don't break.
   bool formatChildren(LineState &State, bool NewLine, bool DryRun,
                       unsigned &Penalty) {
     const FormatToken *LBrace = State.NextToken->getPreviousNonComment();
     FormatToken &Previous = *State.NextToken->Previous;

clang/lib/Format/UnwrappedLineParser.cpp

case tok::kw_asm:
     }
     break;
   case tok::kw_namespace:
     parseNamespace();
     return;
   case tok::kw_public:
   case tok::kw_protected:
   case tok::kw_private:
     if (Style.Language == FormatStyle::LK_Java || Style.isJavaScript() ||
         Style.isCSharp())
       nextToken();
     else
       parseAccessSpecifier();
     return;
   case tok::kw_if:
     if (Style.isJavaScript() && Line->MustBeDeclaration)
       // field/method declaration.
case tok::identifier: {
 
       if (FormatTok->is(Keywords.kw_interface)) {
         if (parseStructLike()) {
           return;
         }
         break;
       }
 
       if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
         parseStatementMacro();
         return;
       }
 
       // See if the following token should start a new unwrapped line.
       StringRef Text = FormatTok->TokenText;
 
       FormatToken *PreviousToken = FormatTok;
if (FormatTok->Tok.is(tok::l_brace)) {
     --Line->Level;
   }
 
   if (Style.RemoveBracesLLVM)
     NestedTooDeep.pop_back();
 }
 
 void UnwrappedLineParser::parseAccessSpecifier() {
+  FormatToken *AccessSpecifierCandidate = FormatTok;
   nextToken();
   // Understand Qt's slots.
   if (FormatTok->isOneOf(Keywords.kw_slots, Keywords.kw_qslots))
     nextToken();
+
+  auto COperatorMatch =
+      std::lower_bound(COperatorsFollowingVar.begin(),
+                       COperatorsFollowingVar.end(), FormatTok->Tok.getKind());
   // Otherwise, we don't know what it is, and we'd better keep the next token.
-  if (FormatTok->Tok.is(tok::colon))
+  if (FormatTok->Tok.is(tok::colon)) {
     nextToken();
     addUnwrappedLine();
+  } else if ((COperatorMatch == COperatorsFollowingVar.end() ||
+              *COperatorMatch != FormatTok->Tok.getKind()) &&
+             !FormatTok->Tok.is(tok::coloncolon)) {
+    // Not a variable name nor namespace name.
+    addUnwrappedLine();
+  } else if (AccessSpecifierCandidate) {
+    // Consider the access specifier to be a C identifier.
+    AccessSpecifierCandidate->Tok.setKind(tok::identifier);
+  }
 }
 
 void UnwrappedLineParser::parseConcept() {
   assert(FormatTok->Tok.is(tok::kw_concept) && "'concept' expected");
   nextToken();
   if (!FormatTok->Tok.is(tok::identifier))
     return;
   nextToken();

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsAccessSpecifiers) {
   verifyFormat("for (Signals signals : f()) {\n}");
   verifyFormat("{\n"
                "  signals.set(); // This needs indentation.\n"
                "}");
   verifyFormat("void f() {\n"
                "label:\n"
                "  signals.baz();\n"
                "}");
+  verifyFormat("private[1];");
+  verifyFormat("testArray[public] = 1;");
+  verifyFormat("public();");
+  verifyFormat("myFunc(public);");
+  verifyFormat("std::vector<int> testVec = {private};");
+  verifyFormat("private.p = 1;");
+  verifyFormat("void function(private...){};");
+  verifyFormat("if (private && public)\n");
+  verifyFormat("private &= true;");
+  verifyFormat("int x = private * public;");
+  verifyFormat("public *= private;");
+  verifyFormat("int x = public + private;");
+  verifyFormat("private++;");
+  verifyFormat("++private;");
+  verifyFormat("public += private;");
+  verifyFormat("public = public - private;");
+  verifyFormat("public->foo();");
+  verifyFormat("private--;");
+  verifyFormat("--private;");
+  verifyFormat("public -= 1;");
+  verifyFormat("if (!private && !public)\n");
+  verifyFormat("public != private;");
+  verifyFormat("int x = public / private;");
+  verifyFormat("public /= 2;");
+  verifyFormat("public = public % 2;");
+  verifyFormat("public %= 2;");
+  verifyFormat("if (public < private)\n");
+  verifyFormat("public << private;");
+  verifyFormat("public <<= private;");
+  verifyFormat("if (public > private)\n");
+  verifyFormat("public >> private;");
+  verifyFormat("public >>= private;");
+  verifyFormat("public ^ private;");
+  verifyFormat("public ^= private;");
+  verifyFormat("public | private;");
+  verifyFormat("public |= private;");
+  verifyFormat("auto x = private ? 1 : 2;");
+  verifyFormat("if (public == private)\n");
+  verifyFormat("void foo(public, private)");
+  verifyFormat("public::foo();");
 }
 
 TEST_F(FormatTest, SeparatesLogicalBlocks) {
   EXPECT_EQ("class A {\n"
             "public:\n"
             "  void f();\n"
             "\n"
             "private:\n"