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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
