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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits