Page MenuHomePhabricator

[clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments
Needs RevisionPublic

Authored by avogelsgesang on Dec 26 2021, 1:19 PM.

Details

Summary

Template argument types can be introduced using either the class or
the typename argument. While there are good arguments for both styles,
usually one wants to use one style consistently across a project.

This commit adds a TemplateArgumentKeyword option which can be used
to format all template arguments consistently using either the class
or the typename keyword.

Implementationwise, it closely follows in the footsteps of D69764, i.e.
it adds a new TemplateArgumentKeywordFixer pass which creates the
appropriate replacements.

Diff Detail

Event Timeline

avogelsgesang created this revision.Dec 26 2021, 1:19 PM
avogelsgesang requested review of this revision.Dec 26 2021, 1:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 26 2021, 1:19 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Fix code formatting

Adding reviewers from D69764 to this change because I think it falls into the same category: It replaces tokens and could thereby potentially break code

MyDeveloperDay added a project: Restricted Project.Dec 27 2021, 12:10 AM

Nice change, some small notes from me.

clang/docs/ClangFormatStyleOptions.rst
4003

Have you edited this by hand? It is on a different (the right) position than in Format.h.

clang/include/clang/Format/Format.h
1963

Please sort (as good as possible, I know some things are out of order).

clang/lib/Format/Format.cpp
144–145

This is what is printed in the documentation, if generated automatically.

1238

Drop this blank line.

clang/lib/Format/TemplateArgumentKeywordFixer.cpp
46

I think you should rename it to TokenKindToReplace or something like that.
A is(BadToken) can be misleading.

50

Should never happen, right? Assert on that.

52

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.

81

assert on non nullness

94

Could be located outside of the loop.

clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
168

Just to be sure add some tests with incomplete code? E.g.

template<typename
avogelsgesang edited the summary of this revision. (Show Details)Dec 27 2021, 11:22 AM
avogelsgesang marked 4 inline comments as done.

Thank you for the quick review, @HazardyKnusperkeks!

I addressed most of your comments by fixing my code.
In two cases, I think there was a misunderstanding and I didn't apply your proposed changes, yet.
See my inline replies re "assert on non nullness" and "typename vs Typename in configuration".
If you still think, I should apply your proposed changes, please let me know. I don't have strong opinions on those points and am happy to do so.

clang/docs/ClangFormatStyleOptions.rst
4003

No, this file is auto-generated using the dump_format_style.py script. Afaict, it sorts the entries alphabetically during generating the docs.

clang/include/clang/Format/Format.h
1963

moved after TabWidth

clang/lib/Format/Format.cpp
144–145

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?

clang/lib/Format/TemplateArgumentKeywordFixer.cpp
46

Good point! Done.

50

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"?

52

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

81

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?

94

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?

clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
168

good point, added a couple of test cases for partial inputs, also including your particular example

Looks nice already. Thanks for working on this.

clang/include/clang/Format/Format.h
3691

Nit.

clang/lib/Format/TemplateArgumentKeywordFixer.cpp
56

Isn't it a better name?

66

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?

70

Please finish comments with full stops, here and below.

76
clang/lib/Format/TemplateArgumentKeywordFixer.h
13

Or even better, make it closer to what's in cpp file.

37

Please make the comments consistent with other files.

clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
88

Second argument can be removed if you add an overload having Expected=Code, so as to match verifyFormat in other files.

156

Don't repeat comments, please. It's the same as below in deduction guides test.

curdeius requested changes to this revision.Dec 28 2021, 4:08 AM
This revision now requires changes to proceed.Dec 28 2021, 4:08 AM
avogelsgesang marked 7 inline comments as done.

Thank you for your review, @curdeius!

I updated my change to address most of your comments.
Regarding "KeepTemplateTemplateKW", I think we had a misunderstanding (see replies to inline comments).
Could you take a 2nd look at those comments?

clang/include/clang/Format/Format.h
3691

Good catch!

I just copied that phrase from QualifierAlignment and overlooked that I also copied the typo. Fixed the typo in both locations.

clang/lib/Format/TemplateArgumentKeywordFixer.cpp
56

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?

66

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>
70

Fixed here and all other places I could find in my change

clang/lib/Format/TemplateArgumentKeywordFixer.h
37

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

clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
156

removed comment in both test cases. Reading it again, I agree that the comment didn't provide much value in the first place

avogelsgesang marked an inline comment as done.

Fix namespace comment

avogelsgesang added inline comments.Dec 28 2021, 9:11 AM
clang/lib/Format/TemplateArgumentKeywordFixer.h
37

ok, now I understood what you meant...
You meant consistency with the other files in this commit, in particular with the corresponding cpp file TemplateArgumentKeywordFixer.h.

Fun fact: I copied this inconsistency from NamespaceEndCommentsFixer.h/.cpp which also use different comment styles between .h and .cpp.

I updated my change to consistently use the // namespace style

Thanks for addressing the comments quickly. I'll have one more thorough look in the coming days.

clang/lib/Format/TemplateArgumentKeywordFixer.cpp
56

I did understand it correctly that it is about class keyword in template template parameters, but my brain somehow melted down the road:). You can keep the name as is.

66

As above, forget my previous comment.

Quuxplusone added inline comments.
clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
119

My personal recommended style is that the programmer uses template<class> consistently and therefore anytime you see the (more arcane) typename keyword it's probably necessary for (more arcane) reasons. You cover some arcane uses of typename above, but I think this one is conspicuously missing:
https://godbolt.org/z/fa9TM4nrr

template<class T, typename T::Nested V>
void f();

You can handle this by looking for :: after the identifier, and if you find it, then typename should be kept as-is. In fact, if you find any token other than , = > after the identifier, something arcane might be happening and it'd be best to keep typename as-is.

Vice versa, template<class C*> can't be rewritten as template<typename C*>; again, I think the right approach is to leave the human's choice alone if you see anything other than , = > after the identifier.

In general, the problems of "Can this typename be rewritten as class" and vice versa are probably undecidable, so don't get too carried away with trying to be perfect as long as you hit all the most common cases anyone can think of. https://quuxplusone.github.io/blog/2019/12/27/template-typename-fun/

clang/lib/Format/TemplateArgumentKeywordFixer.cpp
56

IIUC, it feels like the boolean should be named UseClassKWInTemplateTemplates, and it shouldn't have anything to do with Keeping the human's choice. If the human feeds you some C++17 code using template<template<class> typename T> and asks you to format it as C++14 with TAS_Typename, I think it would be quite appropriate to output template<template<typename> class T>. (Change class to typename because the human asked for TAS_Typename; change typename to class because C++14 implies UseClassKWInTemplateTemplates.)
Anyway, this should have a unit test too.

clang/include/clang/Format/Format.h
1903

Please don't change anything unrelated. You are very welcome to change this in a different patch.

3698

Also unrelated.

clang/lib/Format/Format.cpp
144–145

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. :)

clang/lib/Format/TemplateArgumentKeywordFixer.cpp
81

I meant assert on EndTok.

94

Most likely you are right. That's why I said could not should.

clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
181

Can drop the second argument.

curdeius added inline comments.Dec 29 2021, 4:52 AM
clang/lib/Format/TemplateArgumentKeywordFixer.cpp
56

IIUC, it feels like the boolean should be named UseClassKWInTemplateTemplates, and it shouldn't have anything to do with Keeping the human's choice. If the human feeds you some C++17 code using template<template<class> typename T> and asks you to format it as C++14 with TAS_Typename, I think it would be quite appropriate to output template<template<typename> class T>. (Change class to typename because the human asked for TAS_Typename; change typename to class because C++14 implies UseClassKWInTemplateTemplates.)
Anyway, this should have a unit test too.

56

+1

I think that generally you should change only the cases that match some pattern (e.g. typename/class followed by a single identifier without = nor ::, etc., similarly for template template parameters) instead of changing everything except some cases. This will be much less error-prone.
Also, please add tests as suggested above.

curdeius requested changes to this revision.Jan 3 2022, 3:31 AM
This revision now requires changes to proceed.Jan 3 2022, 3:31 AM

Will you still work on this?

Will you still work on this?

I don't know. I currently have more high-priority work, and don't know whether/when I will find time for this review again.
Also, I realized that I will have to read up more about class-template parameters, e.g. I don't quite understand https://quuxplusone.github.io/blog/2019/12/27/template-typename-fun/, yet.

So, if you are asking to figure out if you should still review this in more detail: No, for the time being you should not.
If you are asking because you would like to take over this commit and finish it so that it can land: Yes, I would be happy to hand this over