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