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