[clang-format] Fix indent for selective formatting.

[Sedeniono]

Summary

Fixes github issues #59178, #58464 and #62799.

The problem was that the LevelIndentTracker remembered
the indentation level of previous deeper levels when
leaving a scope. Afterwards, when it entered again a
deeper level, it blindly reused the previous
indentation level. In case the --lines option was used
such that the previous deeper level was not formatted,
that previous level was whatever happened to be there
in the source code. The formatter simply believed it.

This is fixed by letting the LevelIndentTracker forget
the previous deeper levels when stepping out of them
(=> the added resize() in LevelIndentTracker::nextLine()).
Note that this resize() existed in the past until LLVM
14.0.6, but was changed in https://reviews.llvm.org/D129064
(#56352, rev. 14c30c7) to fix a crash. Our commit here
essentially reverts that crash fix. The crash no longer
occurs, most likely because of the changes made in
https://reviews.llvm.org/D144296 (#60843).

However, just reverting 14c30c7 is not enough, since
then the LevelIndentTracker would forget the current
indent if it happens to encounter a preprocessor
directive on a lower level. Hence, the resize()
in nextLine() may not be executed when handling
a preprocessor directive.

The new test in
FormatTestSelective.ReformatRegionAdjustsIndent check
what originally triggered this patch.
The new tests in
FormatMacroRegardlessOfPreviousIndent check the
formatting of a preprocessor #define. They ensure
that the content of LevelIndentTracker::IndentForLevel
does not affect PP directives.

The new test FormatTestSelective.DontAssert checks
a case where a previous iteration of the present
patch triggered an assert().

See
https://github.com/llvm/llvm-project/issues/59178#issuecomment-1542637781
and https://reviews.llvm.org/D151047
for some more details.

[Sedeniono]

The build failures seem to be unrelated to my changes. The builds of other reviews also show them, and the error messages point to things I haven't changed.

[MyDeveloperDay]

Thank you for the indepth explaination in https://github.com/llvm/llvm-project/issues/59178, that was really helpful for me trying to understand what the problem was.

I think your explaination is clear, the reason why you are suggesting this change, and as it passed all the tests, hopefully we can apply the "Beyonce Rule" on https://github.com/llvm/llvm-project/issues/56352, I recommend we pull in @curdeius as a reviewer as the original author of D129064: [clang-format] Avoid crash in LevelIndentTracker.

I'm going to accept this, but I would ask one of the others gives it the all clear too, let us know if you have commit access and if not and want one of us to land it we'll need you name and email to attribute the change to you.

[Sedeniono]

@MyDeveloperDay Thanks for the positive review.
Regarding https://github.com/llvm/llvm-project/issues/56352 and the Beyoncé Rule, the original fix for the crash actually added a test (see the changes made at that time). The test also still works, since it crashes/debug-asserts with just the reintroduction of the resize(). That caused me to dig some more and I came up with the A.Level = B.Level; part of the fix to fix the crash. The if (!Line.InPPDirective) part was because of another failing test.

Since this is my first submission to llvm, I do not have commit access. So someone else has to commit and push this into main.
Regarding name and mail: If you use "Sedenion" as name and "[e-mail]" as mail, I guess github will attribute the commit to my github account?

Also, should it be merged into the LLVM 16.x branch?

[HazardyKnusperkeks]

Just a side note, please link to commits, or even better the reviews here when talking about old commits. I found it through the github issue, but I think the other way around is better. I was interested in the commit, and only afterwards in what it thought to fix (aka the issue).

[owenpan]

In D151047#4361205, @Sedeniono wrote:

Regarding name and mail: If you use "Sedenion" as name and "[e-mail]" as mail, I guess github will attribute the commit to my github account?

See D150403#4358845.

Also, should it be merged into the LLVM 16.x branch?

+1. @MyDeveloperDay and @HazardyKnusperkeks?

[Sedeniono]

@owenpan Here is the name and mail in the <> format:
Sedenion <[e-mail]>

@HazardyKnusperkeks Ok, I will remember it for the next one.

[HazardyKnusperkeks]

In D151047#4363127, @owenpan wrote:

In D151047#4361205, @Sedeniono wrote:

Also, should it be merged into the LLVM 16.x branch?

+1. @MyDeveloperDay and @HazardyKnusperkeks?

Since it's a regression: yes.

[dyung]

Hi @owenpan and @Sedeniono, this change is causing test failures across many bots, can you take a look and fix or revert if you need time to investigate?

Failing bots:

• https://lab.llvm.org/buildbot/#/builders/139/builds/41339
• https://lab.llvm.org/buildbot/#/builders/216/builds/21703
• https://lab.llvm.org/buildbot/#/builders/247/builds/4864
• https://lab.llvm.org/buildbot/#/builders/164/builds/38364
• https://lab.llvm.org/buildbot/#/builders/188/builds/30101
• https://lab.llvm.org/buildbot/#/builders/197/builds/5761
• https://lab.llvm.org/buildbot/#/builders/184/builds/3948
• https://lab.llvm.org/buildbot/#/builders/245/builds/8785
• https://lab.llvm.org/buildbot/#/builders/258/builds/1288
• https://lab.llvm.org/buildbot/#/builders/249/builds/4662
• https://lab.llvm.org/buildbot/#/builders/230/builds/13432
• https://lab.llvm.org/buildbot/#/builders/109/builds/64778

[Sedeniono]

Ok, sorry, the test failures are on me. There is a lesson learned for me: I naively ran only the formatter unit tests, not the whole test suite of llvm. I somehow assumed that my code changes only affected clang-format, since the UnwrappedLineFormatter is in the "Format" folder. I will need some time to investigate.

[MyDeveloperDay]

I think we need to extract the context of the test from RenameTests to ensure we have it covered here. I don't personally normally run the entire LLVM suite.

[HazardyKnusperkeks]

In D151047#4369503, @MyDeveloperDay wrote:

I think we need to extract the context of the test from RenameTests to ensure we have it covered here. I don't personally normally run the entire LLVM suite.

Dito, and I didn't know (or would have thought about) that clang-tidy uses clang-format code.

[Sedeniono]

Heh, ok, so I wasn't that naive then to not run the tests of everything :-)

I had a look at the issue. The ClangRenameTests first do some replacements, and then call formatAndApplyReplacements() with them, which also formats the lines containing the replacements. With my patch, calling clang-format.exe -style=llvm --lines=13:13 on the following file (line 13 is the OldAlias old_alias;)

#include "header.h"

    namespace x { class Old {}; }
    namespace ns {
    #define REF(alias) alias alias_var;

    #define ALIAS(old) \
      using old##Alias = x::old; \
      REF(old##Alias);

    ALIAS(Old);

    OldAlias old_alias;
    }

triggers the assert in adjustToUnmodifiedLine(). This is what happens in the RenameAliasTest.AliasesInMacros test.

The issue originates already from the line #define REF(alias) alias alias_var;: These are two AnnotatedLine instances at first, namely #define REF(alias) with AnnotatedLine::Level set to 0 and alias alias_var; with AnnotatedLine::Level equals to 1. They get joined eventually. Since my patch sets A.Level = B.Level; in join(), the joined line gets level 1. But the LevelIndentTracker is still in level 0 (i.e. IndentForLevel contains only 1 element).

It is kind of the same problem why I added the if (!Line.InPPDirective) check in nextLine() in the patch: I think, if both AnnotatedLines are PP directives, then join() should not step "into" or "out of" levels. PP-directives are kind-of "temporary insertions". So, replacing in join() the A.Level = B.Level; from my patch with

assert(A.InPPDirective == B.InPPDirective);
if (!A.InPPDirective && !B.InPPDirective)
  A.Level = B.Level;

seems to fix the problem. At least all Google tests (including the ClangRenameTests) pass that didn't fail without the patch. Any opinions on that idea?

I have to think some more about the levels and what it means regarding join(). I also need to get the clang-tidy test to run locally and also add the above example as a formatter test (if possible in a reduced form). Most likely I won't be able to do this until next week, sorry. :-/

To create a new fix, do I understand the guide correctly that I basically execute arc diff --verbatim and mention Depends on D151047 somewhere in the message? Will it then appear here automatically and reopen the review?

[HazardyKnusperkeks]

In D151047#4369742, @Sedeniono wrote:

To create a new fix, do I understand the guide correctly that I basically execute arc diff --verbatim and mention Depends on D151047 somewhere in the message? Will it then appear here automatically and reopen the review?

I don't use arc, I just upload the diffs manually.

[barannikov88]

In D151047#4369742, @Sedeniono wrote:

To create a new fix, do I understand the guide correctly that I basically execute arc diff --verbatim and mention Depends on D151047 somewhere in the message? Will it then appear here automatically and reopen the review?

If you want to create a new review, use arc diff --create. It will always create a new review and assign it a different number.

If you want to reopen review because the patch was reverted:

• cherry-pick the reverted commit;
• do any necessary adjustments for the tests to pass, amend them;
• If you're using arc, use arc diff --update D151047 as usual. It will update the current review, preserving its number.
• If you're not using arc, upload the new diff manually via web interface.

(The --update option is not necessary if the commit message contains URL to this review, but it prevents you from accidentally creating a new review.)

I don't remember if the above steps will change the review status to Reopened.
If that doesn't happen, you can reopen the review manually from the menu below (Add Action... -> Reopen Revision).

[Sedeniono]

Ok, here is the 2nd attempt at fixing the issue. The patch is based on the current main branch.

Regarding LineJoiner::join(): Turns out, not setting A.Level but reverting the original crash fix of https://reviews.llvm.org/D129064 no longer produces the crash. From debugging it I guess this is because of https://reviews.llvm.org/D144296 (although I haven't bisected it to confirm this). I am not really sure whether setting A.Level should or should not be done. So I am keeping it as it is.

But therefore note, since https://reviews.llvm.org/D144296 seems to not have been merged into the LLVM 16, the present fix can probably not be merged without further changes.

So, the new patch basically just reverts the original fix of https://reviews.llvm.org/D129064 (i.e. it re-introduces the resize() in LevelIndentTracker::nextLine()). The other changes are just cosmetics and additional tests.

[Sedeniono]

Fixes github issues #59178, #58464 and #62799.

The problem was that the LevelIndentTracker remembered
the indentation level of previous deeper levels when
leaving a scope. Afterwards, when it entered again a
deeper level, it blindly reused the previous
indentation level. In case the --lines option was used
such that the previous deeper level was not formatted,
that previous level was whatever happened to be there
in the source code. The formatter simply believed it.

This is fixed by letting the LevelIndentTracker forget
the previous deeper levels when stepping out of them
(=> change in LevelIndentTracker::nextLine()).
Note that this used to be the case until LLVM 14.0.6,
but was changed in https://reviews.llvm.org/D129064
(#56352) to fix a crash. Our commit here essentially
reverts that crash fix. It was incorrect/incomplete.

Even with the revert, the crash from
https://reviews.llvm.org/D129064 (#56352) no longer
occurs, most likely because of the changes made in
https://reviews.llvm.org/D144296 (#60843).

The change in adjustToUnmodifiedLine() ensures that
the assert() is only checked if IndentForLevel is
actually accessed.

The new test FormatTestSelective.DontAssert checks
a case where a previous iteration of the present
patch triggered an assert().

The new tests in
FormatMacroRegardlessOfPreviousIndent check the
formatting of a preprocessor #define. They ensure
that the content of LevelIndentTracker::IndentForLevel
does not affect PP directives.

See
https://github.com/llvm/llvm-project/issues/59178#issuecomment-1542637781
for some more details.

This commit is the 2nd try for
https://reviews.llvm.org/D151047

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

[Sedeniono]

Oh damn, I executed arc diff --update D151047 on the main branch instead of my own local branch... Sorry.
Can I somehow revert that? Or should I simply use the "abandon review" action and create a entirely new one?

[barannikov88]

In D151047#4396739, @Sedeniono wrote:

Oh damn, I executed arc diff --update D151047 on the main branch instead of my own local branch... Sorry.
Can I somehow revert that? Or should I simply use the "abandon review" action and create a entirely new one?

Just checkout the local branch and rerun this command :)
You will probably need to reset hard the main branch before or after that.

When you are updating the review and arc requests you to write a message, it asks for "what changed since the last revision".
It would usually be something like "Rebase" or "Address review comments", you don't need to repeat commit message there. You will see the message in the History tab.

[Sedeniono]

Next try at getting the 2nd attempt into review. This is again the complete fix, since the previous revision got reverted.

[Sedeniono]

@barannikov88 ok, thanks. I just did that. The latest diff looks right.
I wondered because phabricator suddenly showed changes in ocaml files, which I certainly did not want.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

80

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.

171–175

And then reformat the comment.

clang/unittests/Format/FormatTestSelective.cpp

663

Have you tried to see what is causing this?

[Sedeniono]

Reformatted comment as requested.

[Sedeniono]

clang/lib/Format/UnwrappedLineFormatter.cpp

80

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?

171–175

Done.

clang/unittests/Format/FormatTestSelective.cpp

663

Yes: It is caused by the ContinueFormatting in UnwrappedLineFormatter::format(), see here. 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 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().

[Sedeniono]

clang/lib/Format/UnwrappedLineFormatter.cpp

171–175

Mh, it says that it couldn't apply the patch. I uploaded a patch containing as only change the reformatted comment, via the Web UI ("Update Diff" at the top right corner). Should I have uploaded a "squashed" diff, containing also ALL changes that I made before (the resize etc)? In other words, is Phabricator expecting individual patches and it merges and accumulates them itself? Or does each new uploaded patch overwrite all previous patches?

[barannikov88]

clang/lib/Format/UnwrappedLineFormatter.cpp

171–175

The latter

[Sedeniono]

Reformatted comment, and submitting it via arc diff --update now.

[Sedeniono]

Next try at getting all changes to phabricator.

[Sedeniono]

clang/lib/Format/UnwrappedLineFormatter.cpp

171–175

Ok, it took me 2 tries, but it seems that I managed to upload the full list of changes via arc diff now. I had to specify the origin commit explicitly.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

80

You could put the intention in a comment and add an assert.

clang/unittests/Format/FormatTestSelective.cpp

663

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.

[Sedeniono]

Updated diff: Captured the intention of "going to lower levels" via a debug assert in nextLine().

[Sedeniono]

clang/lib/Format/UnwrappedLineFormatter.cpp

80

Ok, I put the check into an assert() instead and uploaded a new diff. Note that asserting for Line.Level < IndentForLevel.size() would be wrong, since it can also be equal at this point. The comment already states the intention, so did not change it.

[Sedeniono]

@owenpan , @MyDeveloperDay Any opinion on the latest changes?
Otherwise, since I do not have commit rights, someone needs to commit the changes to main (my name and mail: Sedenion <[e-mail]>). Thanks!

[owenpan]

In D151047#4396727, @Sedeniono wrote:

So, the new patch basically just reverts the original fix of https://reviews.llvm.org/D129064 (i.e. it re-introduces the resize() in LevelIndentTracker::nextLine()). The other changes are just cosmetics and additional tests.

Perhaps we should have two separate patches: the first one that just reverts D129064, and the second for the new tests. That would be much easier to follow, IMO.

clang/lib/Format/UnwrappedLineFormatter.cpp

110

[Sedeniono]

As suggested by @owenpan, I split the commits differently. The first commit now contains only the fix + corresponding tests. The second commit contains refactorings/asserts.
My previous statement (that the whole fix consists of a revert of D129064) was not entirely correct: The fix reintroduces the resize() that got removed in D129064, but it additionally surrounds it by the if (!Line.InPPDirective). Without that if, the formatting is wrong if some PP directives are encountered. (The FormatMacroRegardlessOfPreviousIndent tests check this.)
Therefore, I haven't added one specific commit to the patch that only reverts D129064.

Also, as @owenpan suggested, I replaced the !Line.First->is(tok::comment) with Line.First->isNot(tok::comment) in adjustToUnmodifiedLine().

I also rebased the patch to the current main branch.

[Sedeniono]

If everyone is ok with the changes, could someone commit it for me since I do not have commit rights? Thanks!
Sedenion <[e-mail]>

Note: I cannot really make sense of the error reported by the pre merge checks. I suspect a problem in the build system. I can apply the patch locally on the latest main branch without any conflicts.

[owenpan]

In D151047#4449996, @Sedeniono wrote:

As suggested by @owenpan, I split the commits differently. The first commit now contains only the fix + corresponding tests. The second commit contains refactorings/asserts.

How do I select/download the two commits? I expected two separate reviews after you split the previous patch.

[Sedeniono]

@owenpan Oh wow, you are right. Phabricator shows that there are two separate commits, under "Revision Contents" > "Commits", and their commit messages. But apparently there is no way to download some proper *.patch file that contains the meta data, only the total diff.

So, I uploaded a new diff here (D151047): It contains the bugfix. I will also edit the summary at the top with the corresponding commit message.

I have submitted the refactoring commit separately here: https://reviews.llvm.org/D155094

Note that both patches (D151047 and D155094) are fortunately independent of each other. I checked that both can be applied to the latest main branch.

[owenpan]

clang/unittests/Format/FormatTestSelective.cpp

641–646

I suppose this would make the purpose of the test more clear. However, this new test (in either version) would pass without the patch, so it doesn't capture the bug mentioned in D151047#4369742?

649

Again, these tests would pass without the patch.

658

It's the default and can be deleted.

664

Typo.

[Sedeniono]

clang/unittests/Format/FormatTestSelective.cpp

641–646

When I originally submitted the fix for the incorrect selective formatting and you committed it to the main branch, some clang rename unit tests failed because they ran into some assert() in LevelIndentTracker (see this post), making a revert necessary. There was no test in the format Google Tests themselves that caught that mistake. Hence I added this test. But this also means that this test passes with and without the current patch. See an earlier comment where I describe the problem in more details.

Note that the change in LineJoiner::join() (which triggered the problem) is no longer in the current version of the patch because of some other unrelated changes that happened in the main branch since then. Again, please compare another earlier comment.

649

As above, these tests fail with the original first submitted and incomplete fix. As far as I can remember, it happens if you forget the if (!Line.InPPDirective) condition in the patch. The result is that the PP directives end up being formatted "randomly". No other tests in the formatting test suite so far checked the selective PP directive formatting (at least I got no test failures with the incomplete patch). So yes, the new tests pass with and without the patch, but they are still worthwhile.
Also compare this earlier comment.

658

I think setting it explicitly makes it clearer which setting is the important one that is under test here.

664

I cannot see any typo.

[Sedeniono]

Fixed typo in comment.

[Sedeniono]

clang/unittests/Format/FormatTestSelective.cpp

664

Oh, ok, ditto, not dito. Fixed.

[owenpan]

clang/unittests/Format/FormatTestSelective.cpp

641–646

Then what do you think about changing the test case as suggested?

657

658

Use EXPECT_EQ to make the default explicit.

665–671

681–687

697–703

[Sedeniono]

clang/unittests/Format/FormatTestSelective.cpp

641–646

I now added the hyperlink. Also, added the "format this line only" comment, but on the "}" line, since that is the line that gets formatted. Also changed the length parameter to format() from 1 to 0; this never made any sense to me (a range of 0 length?), but it is done everywhere else, so whatever. Removing the Style.FixNamespaceComments = false; line would be wrong because the value is true for the LLVM style, so I kept it.

[Sedeniono]

Implemented review changes as suggested by @owenpan.

If this is finally ok, someone please commit it to main using the following:
Sedenion <[e-mail]>
Thanks!

[owenpan]

clang/unittests/Format/FormatTestSelective.cpp

643–646

Then I'd remove Style.FixNamespaceComments as a factor by formatting a non-brace line.

[owenpan]

LGTM except for D151047#4508359.

[Sedeniono]

clang/unittests/Format/FormatTestSelective.cpp

643–646

The problem I observed did not trigger by formatting some arbitrary line. It triggered when formatting specifically the line with the closing }. Formatting any other line makes therefore no sense. I just tried it again.
FixNamespaceComments = false means that the expected result is that the code fragment remains unchanged. With FixNamespaceComments = true, the formatter would change it (by adding the // namespace ns comment after the brace).

If you are still offended by the test, I will give up and will remove it. There are still the clang rename tests after all that execute the particular code path and uncovered the problem originally.

[owenpan]

clang/unittests/Format/FormatTestSelective.cpp

643–646

Got it! I didn’t know the assertion was triggered by formatting the closing brace. We should keep the test as is then.

clang/lib/Format/UnwrappedLineFormatter.cpp

if (Style.IndentPPDirectives != FormatStyle::PPDIS_None &&
       unsigned PPIndentWidth =
           (Style.PPIndentWidth >= 0) ? Style.PPIndentWidth : Style.IndentWidth;
       Indent = Line.InMacroBody
                    ? Line.PPLevel * PPIndentWidth +
                          (Line.Level - Line.PPLevel) * Style.IndentWidth
                    : Line.Level * PPIndentWidth;
       Indent += AdditionalIndent;
     } else {
+      // When going to lower levels, forget previous higher levels so that we
+      // recompute future higher levels. But don't forget them if we enter a PP
+      // directive, since these do not terminate a C++ code block.
+      if (!Line.InPPDirective) {
+        assert(Line.Level <= IndentForLevel.size());
+        IndentForLevel.resize(Line.Level + 1);
+      }
       Indent = getIndent(Line.Level);
     }
     if (static_cast<int>(Indent) + Offset >= 0)
       Indent += Offset;
     if (Line.IsContinuation)
       Indent = Line.Level * Style.IndentWidth + Style.ContinuationIndentWidth;
   }
 
if (Line.First->is(tok::comment) && IndentForLevel[Line.Level] != -1)
       return;
     unsigned LevelIndent = Line.First->OriginalColumn;
     if (static_cast<int>(LevelIndent) - Offset >= 0)
       LevelIndent -= Offset;
     IndentForLevel[Line.Level] = LevelIndent;
   }
 
 private:
   /// Get the offset of the line relatively to the level.
   ///
   /// For example, 'public:' labels in classes are offset by 1 or 2
   /// characters to the left from their level.
   int getIndentOffset(const FormatToken &RootToken) {
     if (Style.Language == FormatStyle::LK_Java || Style.isJavaScript() ||
         Style.isCSharp()) {
       return 0;
     }
if (Level == 0)
       return 0;
     return getIndent(Level - 1) + Style.IndentWidth;
   }
 
   const FormatStyle &Style;
   const AdditionalKeywords &Keywords;
   const unsigned AdditionalIndent;
 
   /// 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 of previous non-formatted
   /// lines. Think about the --lines parameter of clang-format.
   SmallVector<int> IndentForLevel;
 
   /// Offset of the current line relative to the indent level.
   ///
   /// For example, the 'public' keywords is often indented with a negative
   /// offset.
   int Offset = 0;
 
   /// The current line's indent.

clang/unittests/Format/FormatTestSelective.cpp

TEST_F(FormatTestSelective, ReformatRegionAdjustsIndent) {
   Style.ColumnLimit = 11;
   EXPECT_EQ("  int a;\n"
             "  void\n"
             "  ffffff() {\n"
             "  }",
             format("  int a;\n"
                    "void ffffff() {}",
                    11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+            "{\n"
+            "return 0;\n"
+            "}\n"
+            "int b()\n"
+            "{\n"
+            "  return 42;\n"
+            "}",
+            format("int a()\n"
+                   "{\n"
+                   "return 0;\n"
+                   "}\n"
+                   "int b()\n"
+                   "{\n"
+                   "return 42;\n" // Format this line only
+                   "}",
+                   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
   Style.IndentWidth = 8;
   Style.UseTab = FormatStyle::UT_Always;
   Style.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   EXPECT_EQ("void f() {\n"
             "\tf();\n"
TEST_F(FormatTestSelective, DontAssert) {
 
   // https://llvm.org/PR56352
   Style.CompactNamespaces = true;
   Style.NamespaceIndentation = FormatStyle::NI_All;
   Code = "\n"
          "namespace ns1 { namespace ns2 {\n"
          "}}";
   EXPECT_EQ(Code, format(Code, 0, 0));
+
+  // https://reviews.llvm.org/D151047#4369742
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+         "#define REF(alias) alias alias_var;\n"
+         "}"; // Format this line only
+  EXPECT_EQ(Code, format(Code, 51, 0));
+}
+
+TEST_F(FormatTestSelective, FormatMacroRegardlessOfPreviousIndent) {
+  // clang-format currently does not (or should not) take into account the
+  // indent of previous unformatted lines when formatting a PP directive.
+  // Technically speaking, LevelIndentTracker::IndentForLevel is only for non-PP
+  // lines. So these tests here check that the indent of previous non-PP lines
+  // do not affect the formatting. If this requirement changes, the tests here
+  // need to be adapted.
+  Style = getLLVMStyle();
+
+  const StringRef Code{"      class Foo {\n"
+                       "            void test() {\n"
+                       "    #ifdef 1\n"
+                       "                #define some\n" // format this line
+                       "         #endif\n"
+                       "    }};"};
+
+  EXPECT_EQ(Style.IndentPPDirectives,
+            FormatStyle::PPDirectiveIndentStyle::PPDIS_None);
+  EXPECT_EQ("      class Foo {\n"
+            "            void test() {\n"
+            "    #ifdef 1\n"
+            "#define some\n" // Formatted line
+            "#endif\n"       // That this line is also formatted might be a bug.
+            "            }};", // Ditto: Bug?
+            format(Code, 57, 0));
+
+  Style.IndentPPDirectives =
+      FormatStyle::PPDirectiveIndentStyle::PPDIS_BeforeHash;
+  EXPECT_EQ("      class Foo {\n"
+            "            void test() {\n"
+            "    #ifdef 1\n"
+            "  #define some\n" // Formatted line
+            "         #endif\n"
+            "    }};",
+            format(Code, 57, 0));
+
+  Style.IndentPPDirectives =
+      FormatStyle::PPDirectiveIndentStyle::PPDIS_AfterHash;
+  EXPECT_EQ("      class Foo {\n"
+            "            void test() {\n"
+            "    #ifdef 1\n"
+            "#  define some\n" // Formatted line
+            "#endif\n" // That this line is also formatted might be a bug.
+            "    }};",
+            format(Code, 57, 0));
 }
 
 } // end namespace
 } // end namespace format
 } // end namespace clang