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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:57
+
+  std::string NewText = " " + Qualifier + " ";
+  NewText += Next->TokenText;
----------------
MyDeveloperDay wrote:
> 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.
Full support for that.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:30
+  // Left to Right ordering requires multiple passes
+  SmallVector<AnalyzerPass, 8> Passes;
+  StringRef &Code;
----------------
MyDeveloperDay wrote:
> 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.
I know what it is. So the 8 is `NumberOfSupportedQualifiers`? My point is that if we add something, let's say attributes ;), one only need to change that constant and both vectors grow accordingly, so that no heap allocation happens.

Nothing I would block that change for.


================
Comment at: clang/unittests/Format/QualifierFixerTest.cpp:573
+  // The Default
+  EXPECT_EQ(Style.QualifierOrder.size(), 5);
+  EXPECT_EQ(Style.QualifierOrder[0], "inline");
----------------
This one would suffice, since we add the values within the test now. But actually I would go for Style.QualifierOrder = {"inline",...}; and no EXPECTs needed.


================
Comment at: clang/unittests/Format/QualifierFixerTest.cpp:855
+
+  Style.TypenameMacros.push_back("HRESULT");
+  Style.TypenameMacros.push_back("DWORD");
----------------
This is not correct, the documentation of TypenameMacros says
```These are expected to be macros of the form:

STACK_OF(...)
```
and HRESULT is just a typedef.
So a test against `STACK_OF(int)` is useful. But the macro detection should be reworked, or maybe dropped?
The handling of `LLVM_NODISARD` basically boils down to handling attributes and `AttributeMacros`.


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

https://reviews.llvm.org/D69764