This is an archive of the discontinued LLVM Phabricator instance.

Add operator style options to clang-format
Needs ReviewPublic

Authored by KitsuneAlex on Jun 8 2023, 6:42 AM.

Details

Summary

First of all, i apologize in advance if anything in the submission proccess was messed up, this is my first time contributing to the LLVM project.

This patch adds a new option 'SpaceAfterOperatorKeyword' to clang-format.
As the name implies, this will insert a space after every operator keyword, effectively splitting off the actual operator from the keyword, which i found is a missing option for my private and professional work.

This patch adds two new options to clang-format;

  • SpaceAfterOperatorOverload, which as the name implies will insert a space after the operator keyword in any operator overload declaration.
  • SpaceAfterOperatorCall, which will insert a space after the operator keyword within operator call expressions or address-of expressions.

This allows more finely grained control over the placement of the actual operator token which follows the operator keyword.

Diff Detail

Event Timeline

KitsuneAlex created this revision.Jun 8 2023, 6:42 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 8 2023, 6:42 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
KitsuneAlex requested review of this revision.Jun 8 2023, 6:42 AM
KitsuneAlex edited the summary of this revision. (Show Details)
KitsuneAlex edited the summary of this revision. (Show Details)Jun 8 2023, 6:45 AM

I made a stupid mistake and messed up the formatting of the anonymous parameter in the unit test code.
Fixing this right now, don't know why it slipped through.

I removed the redundant anonymous parameter in the unit test source string. Locally all formatting tests passed fine.

I took the liberty and extended the unit tests for SpaceAfterTemplateKeyword and SpaceAfterOperator keyword to cover both possible cases, which lead to me discovering that my code was not quite right because of an early-out return path in a calling function.

Everything is fixed and tested now.

craig.topper resigned from this revision.Jun 8 2023, 11:19 AM

Can you add a note to the release notes also please?

clang/include/clang/Format/Format.h
3727

O before T, please resort.

clang/lib/Format/Format.cpp
1737

Not needed, it is initially created with LLVMStyle and then only modified.

clang/unittests/Format/FormatTest.cpp
23025

Please also use a test case with a call to an operator.

Can you add a note to the release notes also please?

Of course! Doing that right now :)

Another thing i was wondering was if it would make sense to add a separate option for deciding whether to insert a space between the operator keyword
specifically in the case of a call expression.

Meaning operator overload definitions would use SpaceAfterOperatorKeyword while operator call expressions would use
something like SpaceAfterOperatorCall maybe. In the case this sounds like a good idea to you, i'd also suggest to have SpaceAfterOperatorCall
use SpaceAfterOperatorKeyword as its default value.

Applied all suggested changes and added a suiting option for aformentioned edge-case for call expressions.
Also added the missing release notes to the apropriate section.

KitsuneAlex retitled this revision from Add SpaceAfterOperatorKeyword style option for clang-format to Add SpaceAfterOperatorKeyword & SpaceAfterOperatorKeywordInCall style options for clang-format.Jun 8 2023, 5:15 PM

Some additional changes because i exceeded the maximum line width on multiple occasions.

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to clang/include/clang/Format/Format.h but does not contain an update to ClangFormatStyleOptions.rst

ClangFormatStyleOptions.rst is generated via clang/docs/tools/dump_format_style.py, please run this to regenerate the .rst

You can validate that the rst is valid by running.

./docs/tools/dump_format_style.py
mkdir -p html
/usr/bin/sphinx-build -n ./docs ./html

I had some issues with Git there because i messed up a merge, so the diff was bad. Here's the proper diff.

Clean up the formatting with clang-format.

MyDeveloperDay added inline comments.Jun 9 2023, 8:52 AM
clang/docs/ClangFormatStyleOptions.rst
4704

could we use something like SpaceAfterOperatorDeclaration to differentiate

4714

I'm not going to lie I'm not a fan of SpaceAfterOperatorKeywordInCall as an option name, any other suggestions for a name?

clang/lib/Format/TokenAnnotator.cpp
3762

kind of unrelated change?

clang/unittests/Format/FormatTest.cpp
22900

I assume I could have foo->operator ==();

MyDeveloperDay added a comment.EditedJun 9 2023, 8:53 AM

I had some issues with Git there because i messed up a merge, so the diff was bad. Here's the proper diff.

NOTE: Clang-Format Team Automated Review Comment

keeps you honest right!

Applied all suggested changes and added a suiting option for aformentioned edge-case for call expressions.
Also added the missing release notes to the apropriate section.

Please mark comments as done, if you think they are done.

clang/docs/ClangFormatStyleOptions.rst
4704

But it also applies to the definition?

Keyword seems to be wrong too, if we have a second option for the calls...

I currently have no recommendations for the naming.

clang/lib/Format/TokenAnnotator.cpp
4205

bool Foo::operator==() = default;?

Please add a test. :)

clang/unittests/Format/FormatTest.cpp
22899

Can you add an empty line, so that the 2 blocks are visually separated?

22900

Of course you can. :)

I had some issues with Git there because i messed up a merge, so the diff was bad. Here's the proper diff.

NOTE: Clang-Format Team Automated Review Comment

keeps you honest right!

Yes, i was referring to the fact that the previous diff was only 30% of my intended changes if you look at the changed files, which was caused by an unsmart local merge on my end.
That's why my following diff corrected the formatting.

KitsuneAlex marked 2 inline comments as done.

This revision of the diff changes the names of the new style options to SpaceAfterOperatorOverload and SpaceAfterOperatorCall, which clarifies their scope a lot better i think.
I also rewrote the logic to apply formatting that fits exactly what their name implies respectively,
the new tests i added should cover all common use cases (except i forgot some again).

This revision of the diff changes the names of the new style options to SpaceAfterOperatorOverload and SpaceAfterOperatorCall, which clarifies their scope a lot better i think.
I also rewrote the logic to apply formatting that fits exactly what their name implies respectively,
the new tests i added should cover all common use cases (except i forgot some again).

Two cases missing: operator==() (Calling within a class), and what happens when taking the address &operator==? (With and without qualification.)

clang/unittests/Format/FormatTest.cpp
22900

Nice to check for that too!

KitsuneAlex retitled this revision from Add SpaceAfterOperatorKeyword & SpaceAfterOperatorKeywordInCall style options for clang-format to Add operator style options to clang-format.Jun 11 2023, 3:16 PM

Revamped the formatting logic again and added the requested unit tests.

KitsuneAlex marked 4 inline comments as done.

Added missing test case for default operator declarations as requested, as well as blank lines between the blocks inside the test functions
as visual separators.

KitsuneAlex edited the summary of this revision. (Show Details)Jun 11 2023, 3:31 PM
KitsuneAlex marked 2 inline comments as done.

Fix broken style in TokenAnnotator.cpp. I don't know how i keep missing these, trying to get better.

did you cover the case where I have a namespaced, static

foo::Bar->operator==()
foo::Bar->m_oper->operator==()
foo::Bar->m_oper.operator==()
foo::Bar.operator==()
clang/lib/Format/TokenAnnotator.cpp
4207–4210

I don't quite understand what case needs this?

clang/lib/Format/TokenAnnotator.cpp
4203
4211

Token could be null here already.

4213

This would be operator::, or not?