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