[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked an inline comment as done.
Sedeniono added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:80
+      // directive, since these do not terminate a code block.
+      if (!Line.InPPDirective && Line.Level < IndentForLevel.size())
+        IndentForLevel.resize(Line.Level + 1, -1);
----------------
HazardyKnusperkeks wrote:
> Do we need this check? I assume `resize` has a check if it has to grow or shrink, so we would check twice when the size is to be changed and once if not.
The `Line.Level < IndentForLevel.size()` is not necessary because of lines 63+64. `resize` itself can both grow and shrink the container. 
If `Line.Level >= IndentForLevel.size()` and we omit the `Line.Level < IndentForLevel.size()` in line 80, then the `resize()` in line 81 does nothing because the container already has the size `Line.Level+1`.

I just added the check in line 80 because I thought it made the intention more clear and explicit: Forget indent levels when going to smaller levels.
Still, I can of course remove it again. Should I?


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:169-170
 
   /// The indent in characters for each level.
+  /// It remembers the indent of previous lines (that are not PP directives) of
+  /// equal or lower levels. This is used to align formatted lines to the indent
----------------
HazardyKnusperkeks wrote:
> And then reformat the comment.
Done.


================
Comment at: clang/unittests/Format/FormatTestSelective.cpp:663
+            "#define some\n" // Formatted line
+            "#endif\n"       // That this line is also formatted might be a bug.
+            "            }};", // Dito: Bug?
----------------
HazardyKnusperkeks wrote:
> Have you tried to see what is causing this?
Yes: It is caused by the `ContinueFormatting` in `UnwrappedLineFormatter::format()`, [see here](https://github.com/llvm/llvm-project/blob/8abbc96885c836de6912a5f4327519c09f4e528a/clang/lib/Format/UnwrappedLineFormatter.cpp#L1332C4-L1335). If that flag is true, clang-format changes the formatting of lines that are not affected by the `--lines` option. It more or less stops this at the next right-brace `}`. I am guessing that `#endif` should be handled like a right-brace there, to stop the formatting, but I am not sure: The "continue formatting" was introduced in [this](https://github.com/llvm/llvm-project/commit/a1036e5d081dd800c71bdbdf858908b99eed03a4) commit (http://reviews.llvm.org/D14105). I don't fully understand the intention there, but I guess it is intended for situations where clang-format inserts new `{...}` blocks? So how does this translate to PP directives?

In any case, I think this is an entirely different issue. The main point of these tests is that the `#define some` line is correct, i.e. that `IndentForLevel` is not used for the PP directives, i.e. that the PP directives are not affected by the indent of `void test()`.


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

https://reviews.llvm.org/D151047