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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a subscriber: asb.
MyDeveloperDay added a comment.

> In D69764#2939037 <https://reviews.llvm.org/D69764#2939037>, @HazardyKnusperkeks wrote:



> I would like to remove the CV, only QualifierOrder.

Oh gosh, I agree here so much with both you and @curdeius , I keep miss spelling the CV and it makes the code noisy... let me rework everything now to remove the CV including all the options,

I get @curdeius point about `Specifier` too... I'm OK about going with a concencus (I'm not married to the idea of just `Qualifier`)

> And please take attributes into account, C++ attributes like [[noreturn]], but also compiler attributes like __attribute__((noreturn)) or __declspec(noreturn), I think it should be possible to add <attributes> to your list.

Oh man you are killing me with good suggestions ;-)....

So this will be hard with the current implementation because `[` `[` `noreturn` `]` `]` are all separate tokens. However maybe if we COULD merge the tokens into 1 token (like we do in FormatTokenLexer) even it it was just temporarily for this pass we maybe could do that.

I think I want to follow @klimek suggestion to give the QualifierAlignmentFixer its own set of proper unit tests to ensure I test its individual function at a lower unit level, rather than just looking at the output.

I'm in no rush, I want to let the RFC have a good amount time for people to read and to gather all the feedback, but I'm encouraged enough to think my efforts won't be wasted and I want to address some of @aaron.ballman  concerns to extend a olive branch in the hope that I could at least partially gain approval.

It needs to wait for the next LLVM weekly in my view, where I'm thinking @asb might give it a mention, and then we should wait too for more feedback, as that could be a different audience.

So if you don't mind all the little and often updates (that's how I tend to work) I think its better I keep beating on this until everyone is happy.

If you are subscriber (as that list seems long) and don't want to listen to all the chatter I won't be offended if you drop off.



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2107
+  * ``CVQAS_Leave`` (in configuration: ``Leave``)
+    Don't change cvqualifier to either Left or Right alignment
+
----------------
curdeius wrote:
> Nit: `cvqualifier` seems ugly, here and below.
I agree, I'm going to begin removal completely...


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:140
+  // We only need to think about streams that begin with const.
+  if (!Tok->is(CVQualifierType)) {
+    return Tok;
----------------
HazardyKnusperkeks wrote:
> More Braces (follow)
Uh! this is my own style bleeding through... its why I need clang-format to remove them for me! ;-)


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:201
+      }
+      Next = Next->Next;
+    }
----------------
curdeius wrote:
> That will access a nullptr if `Next` in the previous `if` was null. Is that possible BTW? Can we add a test for this? Maybe some possibly invalid code that starts has identifier and coloncolon but doesn't have a template opener? (maybe just `const std::Foo`)?
I can't actually get this to produce a nullptr trying various

```
const std::Foo
const std::Foo<
const std::Foo<>
const std::Foo<int
const std::Foo<int>
```

I feel this is because its not going to be a `TT_TemplateOpener` if there isn't both `<` and `>` otherwise its a less than. I'll add some asserts to try and see if it ever happens



================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:24
+class QualifierAlignmentFixer : public TokenAnalyzer {
+  std::string CVQualifier;
+
----------------
HazardyKnusperkeks wrote:
> I would go for only Qualifier.
Do you think everywhere now too? including the options?


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

https://reviews.llvm.org/D69764