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

[Jakub Budiský via Phabricator via cfe-commits]

Budovi planned changes to this revision.
Budovi added inline comments.


================
Comment at: clang/unittests/Format/FormatTest.cpp:17868
+  FormatStyle Style = getLLVMStyle();
+  Style.AccessModifierOffset = 4;
+  Style.AllowShortEnumsOnASingleLine = false;
----------------
Budovi wrote:
> 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.
Confirmed that the bug is present without this patch.
Related bug report: https://bugs.llvm.org/show_bug.cgi?id=46927


================
Comment at: clang/unittests/Format/FormatTest.cpp:17874
+  verifyFormat("class C {\n"
+               "    int i;\n"
+               "};\n",
----------------
MyDeveloperDay wrote:
> 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?
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.


================
Comment at: clang/unittests/Format/FormatTest.cpp:17882-17908
+  {
+    const char *Expected = "struct S {\n"
+                           "  private:\n"
+                           "    class C {\n"
+                           "        int j;\n"
+                           "\n"
+                           "      public:\n"
----------------
MyDeveloperDay wrote:
> Budovi wrote:
> > 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.
> 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.
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`.


================
Comment at: clang/unittests/Format/FormatTest.cpp:17914-17918
+  verifyFormat("enum class E {\n"
+               "  A,\n"
+               "  B\n"
+               "};\n",
+               Style);
----------------
Budovi wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94661