[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


================
Comment at: clang/include/clang/Format/Format.h:1903
   ///  lead to incorrect code formatting due to incorrect decisions made due to
-  ///  clang-formats lack of complete semantic information.
+  ///  clang-format's lack of complete semantic information.
   ///  As such extra care should be taken to review code changes made by the use
----------------
Please don't change anything unrelated. You are very welcome to change this in a different patch.


================
Comment at: clang/include/clang/Format/Format.h:3698
+
+  /// \brief Use ``\r\n`` instead of ``\n`` for line breaks.
+  /// Also used as fallback if ``DeriveLineEnding`` is true.
----------------
Also unrelated.


================
Comment at: clang/lib/Format/Format.cpp:144-145
+    IO.enumCase(Value, "Leave", FormatStyle::TAS_Leave);
+    IO.enumCase(Value, "typename", FormatStyle::TAS_Typename);
+    IO.enumCase(Value, "class", FormatStyle::TAS_Class);
+  }
----------------
avogelsgesang wrote:
> HazardyKnusperkeks wrote:
> > This is what is printed in the documentation, if generated automatically.
> The documentation is currently automatically generated. I used the additional comments in `Format.h` to overwrite the "in configuration" values:
> 
> ```
> /// ...
> TAS_Leave,
> /// ...
> TAS_Typename, // typename
> /// ...
> TAS_Class // class
> ```
> 
> Note the additional comments after `TAS_Typename` and `TAS_Class`. They overwrite the "in configuration" names used by dump_format_style.py. The same trick is used, e.g., for the `LanguageStandard` enum.
> 
> Given that using lowercase `typename` and `class` still allow for auto-generated documentation, do yous still want me to switch to uppercase `Typename`/`Class`?
> 
> Personally, I do prefer lowercase here because the language keywords are also lowercase. Are there any other reasons except for auto-generated documentation why you would prefer uppercase config values?
Okay, didn't know that.

Than I just have my personal opinion that I would use upper case, but you can choose what ever you want. :)


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:80
+    }
+    FormatToken *EndTok = Tok->MatchingParen;
+
----------------
avogelsgesang wrote:
> HazardyKnusperkeks wrote:
> > assert on non nullness
> nullness was checked already in line 75:
> 
> ```
> if (Tok->isNot(TT_TemplateOpener) || !Tok->MatchingParen) {
> ```
> 
> Do you still want me to add this `assert`? Should I maybe restructure the code somehow to make it easier to understand? Or are you fine with leaving it as is?
I meant assert on `EndTok`.


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:93
+      bool StartedDefaultArg = Tok->is(tok::equal);
+      auto advanceTok = [&]() {
+        Tok = Tok->Next;
----------------
avogelsgesang wrote:
> HazardyKnusperkeks wrote:
> > Could be located outside of the loop.
> I agree it could be moved outside the loop. Should it, though?
> 
> Having it inside the loop makes the code slightly easier to read by keeping the definition closer to its usage. And performancewise, I do trust the compiler to do a decent job here.
> 
> Or am I somehow misguided in my judgement here?
Most likely you are right. That's why I said could not should.


================
Comment at: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp:181
+  // `typename`.
+  verifyFormat("template", "template", Style);
+  verifyFormat("template <", "template<", Style);
----------------
Can drop the second argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290