HazardyKnusperkeks added inline comments.
================
Comment at: clang/include/clang/Format/Format.h:1903
/// lead to incorrect code formatting due to incorrect decisions made due to
- /// clang-formats lack of complete semantic information.
+ /// clang-format's lack of complete semantic information.
/// As such extra care should be taken to review code changes made by the
use
----------------
Please don't change anything unrelated. You are very welcome to change this in
a different patch.
================
Comment at: clang/include/clang/Format/Format.h:3698
+
+ /// \brief Use ``\r\n`` instead of ``\n`` for line breaks.
+ /// Also used as fallback if ``DeriveLineEnding`` is true.
----------------
Also unrelated.
================
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);
+ }
----------------
avogelsgesang wrote:
> 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?
Okay, didn't know that.
Than I just have my personal opinion that I would use upper case, but you can
choose what ever you want. :)
================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:80
+ }
+ FormatToken *EndTok = Tok->MatchingParen;
+
----------------
avogelsgesang wrote:
> 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?
I meant assert on `EndTok`.
================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:93
+ bool StartedDefaultArg = Tok->is(tok::equal);
+ auto advanceTok = [&]() {
+ Tok = Tok->Next;
----------------
avogelsgesang wrote:
> 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?
Most likely you are right. That's why I said could not should.
================
Comment at: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp:181
+ // `typename`.
+ verifyFormat("template", "template", Style);
+ verifyFormat("template <", "template<", Style);
----------------
Can drop the second argument.
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
