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