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

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.


================
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
----------------
curdeius wrote:
> Nit.
Good catch!

I just copied that phrase from `QualifierAlignment` and overlooked that I also copied the typo. Fixed the typo in both locations.


================
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);
----------------
curdeius wrote:
> Isn't it a better name?
This flag is actually about the usage of the `class` keyword instead of the `typename` keyword for template-template arguments.
`true` means: "Keep using the `class` instead of the `typename` keyword for template-template arguments."

I think the name `KeepTemplateTypenameKW` is wrong. "[...]TypenameKW = true" would mean "use `typename` instead of `class`" to me, and that's exactly the opposite way around.

As such, I think `KeepTemplateTemplateKW` is in fact the better name. If we want to make it even more explicit, we could also use `KeepTemplateTemplateClassKW`. What do you think?


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:65
+  }
+
+  for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
----------------
curdeius wrote:
> 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?
I think you misunderstood the semantics of `KeepTemplateTypenameKW`? Or did I misunderstand your comment?

For both `KeepTemplateTemplateKW = true` and `KeepTemplateTemplateKW = false`, the loop below still reformats `class` to `typename`.

E.g., for the input
```
template<template<class> class X>
```
and `KeepTemplateTemplateKW = true`, we produce
```
template<template<typename> class X>
```
For the same input with `KeepTemplateTemplateKW = false`, we produce
```
template<template<typename> typename X>
```


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:69
+
+    // Find the first `template` keyword on this line
+    while (Tok && Tok->isNot(tok::kw_template))
----------------
curdeius wrote:
> Please finish comments with full stops, here and below.
Fixed here and all other places I could find in my change


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.h:36
+} // end namespace format
+} // end namespace clang
+
----------------
curdeius wrote:
> Please make the comments consistent with other files.
Consistent with which other files?
My comments here are consistent with `NamespaceEndCommentsFixer.h` and `QualifierAlignmentFixer.h` which I used for reference while writing this change


================
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
----------------
curdeius wrote:
> Don't repeat comments, please. It's the same as below in deduction guides test.
removed comment in both test cases. Reading it again, I agree that the comment didn't provide much value in the first place


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290