[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 a comment.

Nice change, some small notes from me.



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:4003
 
+**TemplateArgumentKeyword** (``TemplateArgumentStyle``) :versionbadge:`clang-format 14`
+  Keyword to use for template arguments (``class`` or ``typename``)
----------------
Have you edited this by hand? It is on a different (the right) position than in `Format.h`.


================
Comment at: clang/include/clang/Format/Format.h:1963
+  /// \version 14
+  TemplateArgumentStyle TemplateArgumentKeyword;
+
----------------
Please sort (as good as possible, I know some things are out of order).


================
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);
+  }
----------------
This is what is printed in the documentation, if generated automatically.


================
Comment at: clang/lib/Format/Format.cpp:1237
   LLVMStyle.SpacesInConditionalStatement = false;
 
+  LLVMStyle.TemplateArgumentKeyword = FormatStyle::TAS_Leave;
----------------
Drop this blank line.


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:45
+  StringRef ReplacementStr;
+  tok::TokenKind BadToken;
+  bool KeepTemplateTemplateKW = false;
----------------
I think you should rename it to TokenKindToReplace or something like that.
A `is(BadToken)` can be misleading.


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:49
+  case FormatStyle::TAS_Leave:
+    return {Fixes, 0};
+  case FormatStyle::TAS_Typename:
----------------
Should never happen, right? Assert on that.


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:51
+  case FormatStyle::TAS_Typename:
+    assert(Style.Standard != FormatStyle::LS_Auto);
+    KeepTemplateTemplateKW = Style.Standard < FormatStyle::LS_Cpp17;
----------------
Why is that? It is certainly possible to have that configuration and we should not assert on possible user input. Either error out or handle it. I think we should be on the safe side an pretend auto is before 17. Add a note to the documentation about that.


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:80
+    }
+    FormatToken *EndTok = Tok->MatchingParen;
+
----------------
assert on non nullness


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:93
+      bool StartedDefaultArg = Tok->is(tok::equal);
+      auto advanceTok = [&]() {
+        Tok = Tok->Next;
----------------
Could be located outside of the loop.


================
Comment at: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp:167
+}
+
+} // namespace
----------------
Just to be sure add some tests with incomplete code? E.g.
```
template<typename
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290