avogelsgesang added inline comments.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:4003
+**TemplateArgumentKeyword** (``TemplateArgumentStyle``)
:versionbadge:`clang-format 14`
+ Keyword to use for template arguments (``class`` or ``typename``)
----------------
HazardyKnusperkeks wrote:
> Have you edited this by hand? It is on a different (the right) position than
> in `Format.h`.
No, this file is auto-generated using the `dump_format_style.py` script.
Afaict, it sorts the entries alphabetically during generating the docs.
================
Comment at: clang/include/clang/Format/Format.h:1963
+ /// \version 14
+ TemplateArgumentStyle TemplateArgumentKeyword;
+
----------------
HazardyKnusperkeks wrote:
> Please sort (as good as possible, I know some things are out of order).
moved after `TabWidth`
================
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);
+ }
----------------
HazardyKnusperkeks wrote:
> This is what is printed in the documentation, if generated automatically.
The documentation is currently automatically generated. I used the additional
comments in `Format.h` to overwrite the "in configuration" values:
```
/// ...
TAS_Leave,
/// ...
TAS_Typename, // typename
/// ...
TAS_Class // class
```
Note the additional comments after `TAS_Typename` and `TAS_Class`. They
overwrite the "in configuration" names used by dump_format_style.py. The same
trick is used, e.g., for the `LanguageStandard` enum.
Given that using lowercase `typename` and `class` still allow for
auto-generated documentation, do yous still want me to switch to uppercase
`Typename`/`Class`?
Personally, I do prefer lowercase here because the language keywords are also
lowercase. Are there any other reasons except for auto-generated documentation
why you would prefer uppercase config values?
================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:45
+ StringRef ReplacementStr;
+ tok::TokenKind BadToken;
+ bool KeepTemplateTemplateKW = false;
----------------
HazardyKnusperkeks wrote:
> I think you should rename it to TokenKindToReplace or something like that.
> A `is(BadToken)` can be misleading.
Good point! Done.
================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:49
+ case FormatStyle::TAS_Leave:
+ return {Fixes, 0};
+ case FormatStyle::TAS_Typename:
----------------
HazardyKnusperkeks wrote:
> Should never happen, right? Assert on that.
yes, this should be unreachable. I added an assert.
Out of curiosity/slightly off-topic: where does clang-format stand re
"defensive programming"? "Fail fast, fail often", or rather "don't trust your
callers and handle as many situations as possible as gracefully as possible"?
================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:51
+ case FormatStyle::TAS_Typename:
+ assert(Style.Standard != FormatStyle::LS_Auto);
+ KeepTemplateTemplateKW = Style.Standard < FormatStyle::LS_Cpp17;
----------------
HazardyKnusperkeks wrote:
> 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.
I was under the false impression that `deriveLocalStyle` in `Format.cpp` would
always replace `auto` by one of the actual versions. I was wrong about that.
Thanks for catching this!
I updated the code and added a test case for this particular case
================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:80
+ }
+ FormatToken *EndTok = Tok->MatchingParen;
+
----------------
HazardyKnusperkeks wrote:
> assert on non nullness
nullness was checked already in line 75:
```
if (Tok->isNot(TT_TemplateOpener) || !Tok->MatchingParen) {
```
Do you still want me to add this `assert`? Should I maybe restructure the code
somehow to make it easier to understand? Or are you fine with leaving it as is?
================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:93
+ bool StartedDefaultArg = Tok->is(tok::equal);
+ auto advanceTok = [&]() {
+ Tok = Tok->Next;
----------------
HazardyKnusperkeks wrote:
> Could be located outside of the loop.
I agree it could be moved outside the loop. Should it, though?
Having it inside the loop makes the code slightly easier to read by keeping the
definition closer to its usage. And performancewise, I do trust the compiler to
do a decent job here.
Or am I somehow misguided in my judgement here?
================
Comment at: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp:167
+}
+
+} // namespace
----------------
HazardyKnusperkeks wrote:
> Just to be sure add some tests with incomplete code? E.g.
> ```
> template<typename
> ```
good point, added a couple of test cases for partial inputs, also including
your particular example
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
