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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

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



================
Comment at: clang/lib/Format/FormatToken.h:125
+/// Operators that can follow a C variable.
+static const std::set<clang::tok::TokenKind> C_OperatorsFollowingVar = {
+    tok::l_square,     tok::r_square,
----------------
HazardyKnusperkeks wrote:
> And maybe choose a different container: https://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc
Not done it seems.
Please rename and use a different type. Maybe `ImmutableSet`? Or just a sorted `std::vector`?


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:109
+        return true;
+      // Handle Qt signals
+      else if ((RootToken.isOneOf(Keywords.kw_signals, Keywords.kw_qsignals) &&
----------------
Nit here and elsewhere: full stop at the end of comments.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:113
+        return true;
+      // Handle malformed access specifier i.e. 'private' without trailing ':'
+      else if ((RootToken.isAccessSpecifier(false) &&
----------------



================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:116
+                (!RootToken.Next ||
+                 (!C_OperatorsFollowingVar.count(
+                      RootToken.Next->Tok.getKind()) &&
----------------
Please use `contains` instead of `count` if using `ImmutableSet`.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2497
 void UnwrappedLineParser::parseAccessSpecifier() {
+  auto *AccessSpecifierCandidate = FormatTok;
   nextToken();
----------------



================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2506-2512
+    // is not a variable name or namespacename
+  } else if (!C_OperatorsFollowingVar.count(FormatTok->Tok.getKind()) &&
+             !FormatTok->Tok.is(tok::coloncolon))
+    addUnwrappedLine();
+  // consider the accessSpecifier to be a C identifier
+  else if (AccessSpecifierCandidate)
+    AccessSpecifierCandidate->Tok.setKind(tok::identifier);
----------------



================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2507
+    // is not a variable name or namespacename
+  } else if (!C_OperatorsFollowingVar.count(FormatTok->Tok.getKind()) &&
+             !FormatTok->Tok.is(tok::coloncolon))
----------------
Ditto. Use `contains`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117416