[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/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