HazardyKnusperkeks accepted this revision. HazardyKnusperkeks added inline comments. This revision is now accepted and ready to land.
================ 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); ---------------- Sedeniono wrote: > 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? You could put the intention in a comment and add an assert. ================ 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? ---------------- Sedeniono wrote: > 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()`. I've seen (and appreciated) that behavior. If you insert for example a `}` and format that line, following lines will be re-indented, so they are not off. Repository: rG LLVM Github Monorepo 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
