Page MenuHomePhabricator

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

Authored by Budovi on Thu, Jan 14, 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

Unit TestsFailed

TimeTest
20 msx64 debian > Clang-Unit.Format/_/FormatTests::FormatTest.IndentAccessModifiers
Note: Google Test filter = FormatTest.IndentAccessModifiers [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
30 msx64 windows > Clang-Unit.Format/_/FormatTests_exe::FormatTest.IndentAccessModifiers
Note: Google Test filter = FormatTest.IndentAccessModifiers [==========] Running 1 test from 1 test case.

Event Timeline

Budovi requested review of this revision.Thu, Jan 14, 2:07 AM
Budovi created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jan 14, 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
17882–17908

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.

17914–17918

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
2039

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

2039

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

clang/unittests/Format/FormatTest.cpp
17868

Why are those non-related style options here?

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

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
17868

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
17874

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?

17882–17908

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.