[PATCH] D144709: [clang-format] Improve west to east const

Björn Schäpers via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 24 12:42:48 PST 2023


HazardyKnusperkeks added a comment.

In D144709#4149737 <https://reviews.llvm.org/D144709#4149737>, @AlexanderHederstaf wrote:

> I tried to refactor QualifierAlignmentFixer to handle more types, notably removing the *,&,&& requirement as e.g. const Foo did not work. To avoid moving qualifiers too far, a test for "already east const" is introduced. **However, to get the correct order for the east qualifiers Clang-Format must be used twice.** I assume it's related to how fixes/replacements are handled. Would you like to have a look and perhaps suggest any improvements?
>
> As it must be used twice, some tests related to volatile/const order fail, but I noticed most of them would also fails previously if the type is not simple.

Highlight by me. That is not acceptable, running the output of `clang-format` through `clang-format` shall not result in any change. In that case it's better to not touch it in any way.



================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:226
+
+  const bool IsEastQualifier = PreviousCheck && [PreviousCheck]() {
+    if (PreviousCheck->is(tok::r_paren)) {
----------------
In the initial commit concerns were raised that `East` and `West` are westcentric terms and should not be used. Thus you should stick here to `Left` and `Right` the same terms we use in the configuration.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:228
+    if (PreviousCheck->is(tok::r_paren)) {
+      return true;
+    } else if (PreviousCheck->is(TT_TemplateCloser)) {
----------------
Could you add comments what kind of source code we would have here?


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:229
+      return true;
+    } else if (PreviousCheck->is(TT_TemplateCloser)) {
+      return PreviousCheck->MatchingParen->Previous->isNot(tok::kw_template);
----------------
No `else` after `return`.


================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:230
+    } else if (PreviousCheck->is(TT_TemplateCloser)) {
+      return PreviousCheck->MatchingParen->Previous->isNot(tok::kw_template);
+    } else if (PreviousCheck->isOneOf(TT_PointerOrReference, tok::identifier,
----------------
That may be null, or not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144709



More information about the cfe-commits mailing list