Page MenuHomePhabricator

[clang-format] [PR19056] Add support for access modifiers indentation
ClosedPublic

Authored by Budovi on Jan 14 2021, 2:07 AM.

Details

Summary

Adds support for coding styles that make a separate indentation level for access modifiers, such as Code::Blocks or QtCreator.

The new option, IndentAccessModifiers, if enabled, forces the content inside classes, structs and unions (“records”) to be indented twice while removing a level for access modifiers. The value of AccessModifierOffset is disregarded in this case, aiming towards an ease of use.

The PR (https://bugs.llvm.org/show_bug.cgi?id=19056) had an implementation attempt by @MyDeveloperDay already (https://reviews.llvm.org/D60225) but I've decided to start from scratch. They differ in functionality, chosen approaches, and even the option name. The code tries to re-use the existing functionality to achieve this behavior, limiting possibility of breaking something else.

Diff Detail

Event Timeline

Budovi requested review of this revision.Jan 14 2021, 2:07 AM
Budovi created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 2:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I would add a test where you have a member before the first access modifier.
Also this is not exactly what they wanted in the bug, as far as I can see members of structs or classes with no access modifier at all should only be indented once.

I would add a test where you have a member before the first access modifier.

The very first unit test has no modifiers, the third one has a member before a modifier in the nested class C.

Also this is not exactly what they wanted in the bug, as far as I can see members of structs or classes with no access modifier at all should only be indented once.

I don't think there is a consensus about how this should actually work, see e. g. https://bugs.llvm.org/show_bug.cgi?id=19056#c11 .

I see qtcreator's style a bit more relevant than some astyle configuration of a specific project. I also find it personally ugly, can't use it myself and thus would have to make it configurable. In that case, this change could be viewed as a "possible stepping stone", even though it would probably require a major overhaul of the logic.

The previous patch, if I read it correctly, implements this detail in the same way as I did.

clang/unittests/Format/FormatTest.cpp
19175–19201

When you re-create this test using verifyFormat, you get a weird results, i.e. if you keep the newlines before access modifiers, the test fails because it thinks they should not be there, but if you remove them, test fails again, because the formatter includes them instead.

It's a good question whether it is a side-effect of test::messUp or a different bug, I'm not sure.

19207–19211

This unit test currently fails, I believe it is a non-related bug in the formatter. Nevertheless I wanted to add a check that enums (non-records) are preserved.

That looks like a strange style option to me. Is there any bigger codebase formatting the code this way?

clang/docs/ClangFormatStyleOptions.rst
2363

The name IndentAccessModifiers is misleading for me. Access modifiers are public, protected, private.

2363

Hmm, actually that's the description that is misleading.

clang/unittests/Format/FormatTest.cpp
19161

Why are those non-related style options here?

Budovi added inline comments.Jan 14 2021, 9:33 AM
clang/docs/ClangFormatStyleOptions.rst
2363

Hm. Will go through the bug report again to see how people describe it. I'm open to suggestions, but I'll need to fix it anyway because I see a grammar mistake.

The current way I tried to go about it is that, without this option, modifiers don't have their own indentation level. Without the offset given by the AccessModifierOffset they would just be flush with the record members.

The introduced option creates a level designated for the access modifiers by shifting the members further to the right.

clang/unittests/Format/FormatTest.cpp
19161

AccessModifierOffset tests whether it has an influence, even thought now realizing that the default value (-2) is non-zero and is probably good enough as arbitrary 4. So I actually want a non-zero value for it, and this was not obvious at first.

AllowShortEnumsOnASingleLine is needed unless I want to make the enum more complex.

BreakBeforeBraces along with BraceWrapping deals with the chosen enum test below, which had surprising defaults for me in the LLVMStyle. I've added them as a preliminary way of "resolving" the issue with the unit test but it still fails. I agree that the unit test should not try to cover the other features and I will remove them and fix the test as soon as I get into it and confirm that the failing test is not a bug introduced with this change and open up a bug report for it.

clang/unittests/Format/FormatTest.cpp
19167

So this is indented 4 because the Access modifier is 4? when the IndentWidth is 2?

but public below is indented 2.. so now I'm really confused?

19175–19201

As a drive by comment, its my experience when verifyFormat fails its often a sign that there is a bug which means clang-format cannot always format from arbitrary text, which really it should.

Ultimately this bug will get logged by someone who doesn't understand your change as well as you do. That fix will also subtly end up creating a different bug in another corner case you you haven't covered in your original tests

We spiral down from there...my 2c worth.

Budovi planned changes to this revision.Tue, Feb 2, 9:12 AM
Budovi added inline comments.
clang/unittests/Format/FormatTest.cpp
19161

Confirmed that the bug is present without this patch.
Related bug report: https://bugs.llvm.org/show_bug.cgi?id=46927

19167

The indentation is 2 levels below the class since the single level is reserved for access modifiers.

Access modifier does not influence the behaviour at all. The value was chosen arbitrarily, see my previous comment. Will reflect this in an updated patch.

19175–19201

This issue was caused by an inconsistency with access modifiers and unrelated to this patch. After the test::messUp all the newlines are stripped from the code, and clang-format behaved differently in a case there was no newline before access modifier (inserted a single one) and all the other cases (it inserted an additional empty line before the modifier).

The issue was fixed along with a new feature introduced https://reviews.llvm.org/D93846 and thus does not need reporting. The new patch will revert back to using verifyFormat.

19207–19211

As mentioned above, this is the bug https://bugs.llvm.org/show_bug.cgi?id=46927.
The unrelated style options will be removed in the future patch and the "default LLVM behaviour" tested.

Budovi updated this revision to Diff 320818.Tue, Feb 2, 9:30 AM

Updated the tests after checking the issues were not caused by the patch and they are reported / fixed.

Rebased the changes to the newest version.

You need to supply a full diff (with context).
Please also add it to the release notes.

Budovi planned changes to this revision.Tue, Feb 2, 12:09 PM

You need to supply a full diff (with context).
Please also add it to the release notes.

Oh sorry, forgot about the context lines. Will try to fix it tomorrow, along with the release notes.

Budovi updated this revision to Diff 321392.Thu, Feb 4, 5:23 AM

Changes:

  • Added release notes
  • Changed the documentation (tried to make it more clear in response to the confusion about the added option)

Hopefully there is a full context in the patch this time.

Budovi added inline comments.Thu, Feb 4, 5:25 AM
clang/docs/ClangFormatStyleOptions.rst
2363

Hopefully the description is better this time.

curdeius requested changes to this revision.Thu, Feb 4, 6:38 AM

We're getting close :).

clang/unittests/Format/FormatTest.cpp
19161

Please add an assertion that IndentWidth == 2 to make the test easier to understand.

19191

Add a comment that enums are not concerned.

19204

Add tests with a different IndentWidth.

This revision now requires changes to proceed.Thu, Feb 4, 6:38 AM

Don't forget to mark comments as Done when... they're done :).

clang/docs/ClangFormatStyleOptions.rst
2363

Much better indeed! Thanks!

Budovi planned changes to this revision.Fri, Feb 5, 12:21 AM
Budovi added inline comments.
clang/unittests/Format/FormatTest.cpp
19161

The written tests rely on several other "defaults" from the LLVM style. If somebody changes those defaults these unit tests (along with many others) will fail. This will also be true if an assertion is present to test this condition. It would be possible to reassign those values but this goes against how the rest of the tests are implemented.

But I agree that I could improve the readability by adding brief comments that point out important settings and the purpose of the included tests.

Budovi updated this revision to Diff 321724.Fri, Feb 5, 5:57 AM

Changes:

  • Added a unit test with a different IndentWidth
  • Removed a unit test that contained a class nested in function; didn't add much IMHO
  • Added brief comments about potentially non-obvious behaviour to some tests

It's getting into a good shape. Please fix the nits.

clang/unittests/Format/FormatTest.cpp
19164

Nit: please put full stops at the end of comments. Here and elsewhere.

19202

Typo: agnostic.

Budovi updated this revision to Diff 322645.Wed, Feb 10, 4:24 AM

Changes:

  • Fixed comments

Thanks @curdeius for pointing out the issues.

curdeius accepted this revision.Wed, Feb 10, 5:19 AM

LGTM. But let other folks a few days to chime in. Especially @MyDeveloperDay's input would be valuable here as he already attempted to implement this.
Do you have have commit rights or you need somebody to land this for you? If it's the latter, please provide "Firstname Name <mail@domain>" for the commit attribution or we will just use the one associated with your Phabricator user.

This revision is now accepted and ready to land.Wed, Feb 10, 5:19 AM

LGTM. But let other folks a few days to chime in. Especially @MyDeveloperDay's input would be valuable here as he already attempted to implement this.

Sure, no problem. Thanks for your input once again :), really appreciate it.

Do you have have commit rights or you need somebody to land this for you? If it's the latter, please provide "Firstname Name <mail@domain>" for the commit attribution or we will just use the one associated with your Phabricator user.

I don't have commit rights (this is my first contribution… well an attempt anyway) but the Phabricator user settings, i.e. something along Jakub Budiský <jakub.budisky@gmail.com>, should be fine.

MyDeveloperDay accepted this revision.Sat, Feb 13, 5:28 AM

the LGTM, please mark your comments as done when done.