[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:28
+
+static void replaceToken(const SourceManager &SourceMgr,
+                         tooling::Replacements &Fixes,
----------------
HazardyKnusperkeks wrote:
> Out of curiosity, on what premise do you choose a static member function that is only used in this file or a local free function? I would always choose the latter (with an anon namespace), to keep the header smaller.
I've been pulled up on using anon namespaces before in previous reviews, personally I always use them myself in my code. To be honest I've been moving most of these functions into actual class statics functions so I can test the functions explicitly in the unit tests (anon namespaces aren't good for that obvs)

I'd like to leave them as statics so I can more easily do that move and add more tests if I find issues. 


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:57
+
+  std::string NewText = " " + Qualifier + " ";
+  NewText += Next->TokenText;
----------------
HazardyKnusperkeks wrote:
> Does not need to be addressed here, but does LLVM have a string builder or so? This produces a lot of (de-)allocations.
I'm not a massive fan of this either, but I'm unsure if LLVM has anything, I think getting functional first is important, we can go for fast if someone can point out a better pattern.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:129
+
+bool LeftRightQualifierAlignmentFixer::isQualifierOrType(
+    const FormatToken *Tok) {
----------------
HazardyKnusperkeks wrote:
> I would prefer to match the order of functions in the header with the order in the cpp.
I can spin that around, (its going to mess with review comments, but I know what you mean)


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:347
+                           QualifierToken);
+      else if (Alignment == FormatStyle::QAS_Left)
+        Tok = analyzeLeft(SourceMgr, Keywords, Fixes, Tok, Qualifier,
----------------
HazardyKnusperkeks wrote:
> Only else and assert? It does nothing if Alignment is something different, or?
You know this was my bad because I reused the alignment type  because it had the left and right but this could be a boolean  as its different from the QualifierAlignment, let me change this for clarity


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:366
+  // Split the Order list by type and reverse the left side
+  bool left = true;
+  for (const auto &s : Order) {
----------------
HazardyKnusperkeks wrote:
> Couldn't you just
> ```
> LeftOrder.assign(std::reverse_iterator(type) /*maybe with a next or prev, haven't thought too much about it*/, Order.rend());
> RightOrder.assign(std::next(type), Order.end());
> ```
> ?
There is probably a better way but it needs to handle "type" being the first or last element, I tried and it didn't quite work.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:24
+
+typedef std::function<std::pair<tooling::Replacements, unsigned>(
+    const Environment &)>
----------------
HazardyKnusperkeks wrote:
> I don't know what the LLVM style is on that, but I prefer `using` anytime over `typedef`.
I was actually just following what is in Format.cpp


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:30
+  // Left to Right ordering requires multiple passes
+  SmallVector<AnalyzerPass, 8> Passes;
+  StringRef &Code;
----------------
HazardyKnusperkeks wrote:
> Has the 8 some meaning? Then maybe give it a name. Or is it just coincidence that you repeat the 8 for QualifierTokens?
For SmallVector this is basically a "ShortStringOptimization" for vector i.e. its stack allocated until the vector goes over 8, I have 7 qualifiers, and I wanted it an order of 2 so hence 8 (its shouldn't grow (and heap allocation) unless we define more qualifier types (this only supports what I say for now)

I think the use of a literal is quite common in this case, its really just a hint, I think its ok to use without it being a variable.


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

https://reviews.llvm.org/D69764