[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

Manuel Klimek via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 20 03:51:44 PDT 2019


klimek added inline comments.


================
Comment at: lib/Format/WhitespaceManager.cpp:436
+static void
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+                      F &&Matches,
----------------
VelocityRa wrote:
> klimek wrote:
> > I don't think the 'All' postfix in the name is helpful. What are you trying to say with that name?
> I'm not particularly fond of `All` either, suggestions welcome.
> 
> As the comment above explains, `All` refers to the fact that it operates on all tokens, instead of being limited to certain cases like `AlignTokenSequence` is. Maybe I should name //this// one `AlignTokenSequence` and the other one `AlignTokenSequenceOuterScope`, or something.
How about calling this AlignMacroSequence and the other one AlignMacros.
Comment here would be something like (as I don't think "scope" plays a role??)
// Aligns a sequence of macro definitions.

The problem is that I think the alignTokensAll don't make sense either as a function.
We should be able to inline this into alignConsecutiveMacros, and pull the std::function handed in either into its own function, or just define it as the first step in alignConsecutiveMacros.


================
Comment at: lib/Format/WhitespaceManager.cpp:437
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+                      F &&Matches,
+                      SmallVector<WhitespaceManager::Change, 16> &Changes) {
----------------
VelocityRa wrote:
> klimek wrote:
> > Why an rvalue ref? Also, this is used only once, so why make this a template?
> It's an rvalue ref, because that's also the case for `AlignTokenSequence` (wasn't sure why, so I left it as is).
> It's used only once, but the function is more generic that way, no? That's the point of its generic name.
> Tell me if I should change it.
I think we need to look at this from first principles. We can fix the old code later if it doesn't make sense :)


================
Comment at: lib/Format/WhitespaceManager.cpp:442
+
+  for (unsigned i = Start; i != End; ++i) {
+    if (Changes[i].NewlinesBefore > 0) {
----------------
VelocityRa wrote:
> klimek wrote:
> > llvm style: use an upper case I for index vars.
> Ok, I assume your style changed because this is copied from `AlignTokenSequence`.
Yea, sorry, those are in violation (we should fix that at some point, but *not* in this patch :)



================
Comment at: lib/Format/WhitespaceManager.cpp:500
+    if (Changes[i].NewlinesBefore != 0) {
+      EndOfSequence = i;
+      // If there is a blank line, or if the last line didn't contain any
----------------
VelocityRa wrote:
> klimek wrote:
> > Why set EndOfSequence outside the if below?
> It's from `AlignTokens`. I think it's because due to some of the loop logic, it ends up not checking up to the point of the the last token.
> Without setting this and calling `AlignCurrentSequence()` once more at the end, the last line of a macro group does not get properly aligned, the tests fail.
I was suggesting to move it inside the if below, did you try that (sounds like you tried to remove it).


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

https://reviews.llvm.org/D28462





More information about the cfe-commits mailing list