[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2039
 
+**IndentAccessModifiers** (``bool``)
+  Makes an indentation level for record (``class``, ``struct``, ``union``)
----------------
curdeius wrote:
> curdeius wrote:
> > The name `IndentAccessModifiers` is misleading for me. Access modifiers are public, protected, private.
> Hmm, actually that's the description that is misleading.
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.


================
Comment at: clang/unittests/Format/FormatTest.cpp:17868
+  FormatStyle Style = getLLVMStyle();
+  Style.AccessModifierOffset = 4;
+  Style.AllowShortEnumsOnASingleLine = false;
----------------
curdeius wrote:
> Why are those non-related style options here?
`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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661