[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:359
 
+- Option ``ConstPlacement`` has been added auto-arrange the positioning of const
+  in variable and parameter declarations to be ``Right/East`` const or ``Left/West`` 
----------------
It sounds strange as if you wanted to write "[in order] to auto-arrange".


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:83
+  // Change `const int` to be `int const`.
+  std::string NewType;
+  NewType += Second->TokenText;
----------------
Nit: `NewType` may be misleading when reading. Why not `NewText` as above?


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:130
+
+static void swapFourTokens(const SourceManager &SourceMgr,
+                                 tooling::Replacements &Fixes,
----------------
These functions seem a bit ugly... and very specific. And they both look like rotate left/right. Couldn't it be a single function taking a range/span/collection of FormatTokens?


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:136
+                                 const FormatToken *Fourth, 
+                                 bool West) {
+  // e.g. Change `const unsigned long long` to be `unsigned long long const`.
----------------
Nit: West doesn't seem appropriate as a name at the level of this function as you rather don't move elements west/east but left/right. Of course, that applies only if you share my view that it's sort of rotate algorithm.


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:171
+    return false;
+  return (Tok->isSimpleTypeSpecifier() ||
+          Tok->isOneOf(tok::kw_volatile, tok::kw_auto));
----------------
Parentheses seem to be unnecessary.


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:171
+    return false;
+  return (Tok->isSimpleTypeSpecifier() ||
+          Tok->isOneOf(tok::kw_volatile, tok::kw_auto));
----------------
curdeius wrote:
> Parentheses seem to be unnecessary.
Stupid question, are both const and restrict handled here?


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:175
+
+// If a token is an identifier and its upper case it could
+// be a macro and hence we need to be able to ignore it
----------------
Typo: its -> it's.
Missing comma before "it could".


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:176
+// If a token is an identifier and its upper case it could
+// be a macro and hence we need to be able to ignore it
+static bool isPossibleMacro(const FormatToken *Tok)
----------------
Missing dot at the end.


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:195
+                                FormatToken *Tok) {
+  // We only need to think about streams that begin with const.
+  if (!Tok->is(tok::kw_const)) {
----------------
Why? What about `unsigned const int`?


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:208
+
+  if (isCVQualifierOrType(Tok->Next) && isCVQualifierOrType(Tok->Next->Next) && isCVQualifierOrType(Tok->Next->Next->Next)) {
+    swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next->Next, Tok->Next->Next->Next,
----------------
I think that this code asks for a cleanup. And if we needed to look for five tokens...?


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:213
+  }
+  if (isCVQualifierOrType(Tok->Next) && Tok->Next->Next &&
+      isCVQualifierOrType(Tok->Next->Next)) {
----------------
Shouldn't it be `else if`?


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:223
+             Tok->Next->is(tok::kw_auto)) {
+    // The basic case  `const int` -> `int const`
+    swapTwoTokens(SourceMgr, Fixes, Tok, Tok->Next);
----------------
Nits: double space, missing ending dot.


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:225
+    swapTwoTokens(SourceMgr, Fixes, Tok, Tok->Next);
+
+  } else if (Tok->startsSequence(tok::kw_const, tok::identifier,
----------------
Nit: unnecessary blank line.


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:228
+                                 TT_TemplateOpener)) {
+    // Read from to the end of the TemplateOpener to
+    // TemplateCloser const ArrayRef<int> a; const ArrayRef<int> &a;
----------------
"from to the end"?


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:244
+      removeToken(SourceMgr, Fixes, Tok);
+      //return EndTemplate->Next;
+      return Tok;
----------------
Forgotten remnants?


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:383
+    while (Tok && Tok != Last) {
+      if (!Tok->Next) {
+        break;
----------------
It's a matter of taste, but this condition could be moved into the while condition above.


================
Comment at: clang/lib/Format/EastWestConstFixer.cpp:387
+      if (Tok->is(tok::comment)) {
+        Tok = Tok->Next;
+        continue;
----------------
You would avoid repetition of this statement if you changed while loop into for loop.


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

https://reviews.llvm.org/D69764