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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Looks nice already. Thanks for working on this.



================
Comment at: clang/include/clang/Format/Format.h:3691
+  ///  COULD lead to incorrect code formatting due to incorrect decisions made
+  ///  due to clang-formats lack of complete semantic information. As such extra
+  ///  care should be taken to review code changes made by the use of this
----------------
Nit.


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:55
+    // For `auto` language version, be conservative and assume we are < C++17
+    KeepTemplateTemplateKW = (Style.Standard == FormatStyle::LS_Auto) ||
+                             (Style.Standard < FormatStyle::LS_Cpp17);
----------------
Isn't it a better name?


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:65
+  }
+
+  for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
----------------
Could we check `KeepTemplateTypenameKW` and early return here?
The optimizer might do some job, but I doubt it will get rid of the unnecessary finding of template keyword etc.
We could maybe avoid this variable altogether and return inside the switch, no?


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:69
+
+    // Find the first `template` keyword on this line
+    while (Tok && Tok->isNot(tok::kw_template))
----------------
Please finish comments with full stops, here and below.


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:75
+
+    // Find the corresponding `<`. Might be on the next line
+    Tok = Tok->Next;
----------------



================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.h:12
+/// This file declares TemplateArgumentKeywordFixer, a TokenAnalyzer that
+// /enforces / consistent usage of `typename`/`class` for template arguments.
+///
----------------
Or even better, make it closer to what's in cpp file.


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.h:36
+} // end namespace format
+} // end namespace clang
+
----------------
Please make the comments consistent with other files.


================
Comment at: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp:87
+  // `typename` outside of a template should stay unchanged
+  verifyFormat("typename T::nested", "typename T::nested", Style);
+  // `typename` inside a template should stay unchanged if it refers to a nested
----------------
Second argument can be removed if you add an overload having Expected=Code, so as to match `verifyFormat` in other files.


================
Comment at: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp:155
+  // We also format template argument lists for `using`.
+  // There are no special semantics re. deduction guides and the normal
+  // implementation just works. But better test it before this breaks due to
----------------
Don't repeat comments, please. It's the same as below in deduction guides test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290