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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
