[clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[goldstein.w.n]

Summary

I.e with:
PPIndentWidth = 1
IndentWidth = 4

#ifdef foo
#define bar() if (A) { B(); } C();
#endif

Assume ColumnLimit Here: |

IndentPPDirectives: None

#ifdef foo
#define bar()               \
    if (A) {                \
        B();                \
    }                       \
    C();                    \
#endif

As opposed to

#ifdef foo
#define bar()               \
  if (A) {                  \
   B();                     \
  }                         \
  C();
#endif

IndentPPDirectives: AfterHash

#ifdef foo
# define bar()              \
    if (A) {                \
        B();                \
    }                       \
    C();                    \
#endif

As opposed to

#ifdef foo
# define bar()              \
  if (A) {                  \
   B();                     \
  }                         \
  C();
#endif

IndentPPDirectives: BeforeHash

#ifdef foo
 #define bar()              \
    if (A) {                \
        B();                \
    }                       \
    C();                    \
#endif

As opposed to

#ifdef foo
 #define bar()              \
  if (A) {                  \
   B();                     \
  }                         \
  C();
#endif

[MyDeveloperDay]

This needs a unit test in clang/unittest/Format

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Added unit tests.

[owenpan]

Can you fix the format of the summary?

clang/unittests/Format/FormatTest.cpp

5043

Don't add an empty line here.

5063

Do you need it?

5065

Do you need to enclose the macro definition in #ifdef/#endif?

5072–5074

You can delete them.

5084–5086

Delete them or change to multiline format like lines 5048-5052.

5096–5098

Same here.

[goldstein.w.n]

clang/unittests/Format/FormatTest.cpp

5043

Will drop for V2.

5063

No, copied along when I was looking for examples of unit-tests with trailing '\'. I'll drop for V2

5065

No, but figured since this change is related to PP indentation the tests should have it. You would prefer I make the test just:

verifyFormat("#define bar() \\\n"
             "    if (A) {  \\\n"
             "        B();  \\\n"
             "    }         \\\n"
             "    C();\n",
             "#define bar() if (A) { B(); } C();\n",
             style);

?

5072–5074

Them being the ifdef/endif? If so will do for V2.

5084–5086

Delete the test of the ifndef/endif? Can do either for V2.

5096–5098

Same question as above, otherwise will do for V2.

[goldstein.w.n]

In D137181#3903161, @owenpan wrote:

Can you fix the format of the summary?

I see the misindentation of the After/Before hash examples. Will fix those for V2.
Anything else?

[owenpan]

clang/unittests/Format/FormatTest.cpp

5065

My bad. Please ignore my comment.

5072–5074

Them being lines 5054-5056. (If you mouse over the comment, the lines will be highlighted.)

5084–5086

Actually, delete lines 5066-5068. There are mainly two ways to use verifyFormat: verifyFormat(Expected, Input, ...) and verifyFormat(Input, ...). Here we can use the latter, in which Input is also expected output.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

1. Fixed indentation in commit
2. Fixed format nits in tests
3. Remove initial code from tests

[goldstein.w.n]

In D137181#3899874, @MyDeveloperDay wrote:

This needs a unit test in clang/unittest/Format

Fixed in V2 I think but not sure as the summary doesn't seem to have changed.

clang/unittests/Format/FormatTest.cpp

5072–5074

Got it. Dropped in V2.

5084–5086

Got it. Dropped in V2.

[owenpan]

clang/unittests/Format/FormatTest.cpp

5075–5078

I just noticed that here and below you got an extra IndentWidth than in the summary, so the patch only works for PPDIS_None?

[goldstein.w.n]

clang/unittests/Format/FormatTest.cpp

5075–5078

To a degree.

Level is used by both scope depth and PP depth so nested PP directives with before/after will essentially have IndentWidth * (PPLevel + ScopeLevel) as net indentation.

AFAICT UnwrappedLineParser.cpp::parsePPDefine needs to something other than Line->Level with PBBranchLevel + 1.

I started doing that but the diff got a bit bigger given that it needs to get propegated between Wrapper/Unwrapped/Annotated (AFAICT).

Would you prefer its implemented like that?

[owenpan]

With ColumnLimit: 16 and IndentPPDirectives: BeforeHash, the format should be:

#ifdef foo
 #define bar() \
     if (A) {  \
         B();  \
     }         \
     C();
#endif

With IndentPPDirectives: AfterHash, it should look like:

#ifdef foo
# define bar() \
     if (A) {  \
         B();  \
     }         \
     C();
#endif

[owenpan]

clang/unittests/Format/FormatTest.cpp

5075–5078

See D137181#3904383

[goldstein.w.n]

clang/unittests/Format/FormatTest.cpp

5075–5078

Hmm? What is that suppose to link to?

[goldstein.w.n]

clang/unittests/Format/FormatTest.cpp

5075–5078

Err, sorry I see now. Got it.

[goldstein.w.n]

In D137181#3904383, @owenpan wrote:

With ColumnLimit: 16 and IndentPPDirectives: BeforeHash, the format should be:

#ifdef foo
 #define bar() \
     if (A) {  \
         B();  \
     }         \
     C();
#endif

With IndentPPDirectives: AfterHash, it should look like:

#ifdef foo
# define bar() \
     if (A) {  \
         B();  \
     }         \
     C();
#endif

Doesn't that add an arbitrary +1 to the begining of the indentation?
Shouldn't it be:

// IndentPPDirectives: AfterHash
#ifdef foo
# define bar() \\
  if (A) {     \\
      B();     \\
  }            \\
  C();
#endif

// IndentPPDirectives: BeforeHash
#ifdef foo
 #define bar() \\
  if (A) {     \\
      B();     \\
  }            \\
  C();
#endif

// IndentPPDirectives: NoneHash
#ifdef foo
#define bar() \\
 if (A) {     \\
     B();     \\
 }            \\
 C();
#endif

[owenpan]

In D137181#3910404, @goldstein.w.n wrote:

Doesn't that add an arbitrary +1 to the begining of the indentation?
Shouldn't it be:

// IndentPPDirectives: AfterHash
#ifdef foo
# define bar() \\
  if (A) {     \\
      B();     \\
  }            \\
  C();
#endif

// IndentPPDirectives: BeforeHash
#ifdef foo
 #define bar() \\
  if (A) {     \\
      B();     \\
  }            \\
  C();
#endif

// IndentPPDirectives: NoneHash
#ifdef foo
#define bar() \\
 if (A) {     \\
     B();     \\
 }            \\
 C();
#endif

Given the settings used in your example:

PPIndentWidth: 1
IndentWidth: 4

IMO the macro body should be shifted to the right by 1 column (except when IndentPPDirectives is set to None). That is, the indent of the macro body relative to the start of define should be the same with any setting of IndentPPDirective.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Make it so that code indentation always starts where 'd' in "define" is.

[goldstein.w.n]

In D137181#3910566, @owenpan wrote:

In D137181#3910404, @goldstein.w.n wrote:

Doesn't that add an arbitrary +1 to the begining of the indentation?
Shouldn't it be:

// IndentPPDirectives: AfterHash
#ifdef foo
# define bar() \\
  if (A) {     \\
      B();     \\
  }            \\
  C();
#endif

// IndentPPDirectives: BeforeHash
#ifdef foo
 #define bar() \\
  if (A) {     \\
      B();     \\
  }            \\
  C();
#endif

// IndentPPDirectives: NoneHash
#ifdef foo
#define bar() \\
 if (A) {     \\
     B();     \\
 }            \\
 C();
#endif

Given the settings used in your example:

PPIndentWidth: 1
IndentWidth: 4

IMO the macro body should be shifted to the right by 1 column (except when IndentPPDirectives is set to None). That is, the indent of the macro body relative to the start of define should be the same with any setting of IndentPPDirective.

Okay set it up so that where the 'd' in "define" is is always where indentation starts.
Essentially track PPLevel independently of Level and use the two seperarely.

I probably missed a few places / added in a few bad places.

[owenpan]

IMO we should find a simpler way to indent bodies of macro definitions that are nested more than one level deep. I prefer we handle that in another patch and only support the examples in the summary for now.

[goldstein.w.n]

In D137181#3910799, @owenpan wrote:

IMO we should find a simpler way to indent bodies of macro definitions that are nested more than one level deep. I prefer we handle that in another patch and only support the examples in the summary for now.

I'm not sure what changes to the patch that would imply?
Also want to note, for the unit tests to all pass only 4-places actually need the
PPLevel tracking.
L373
L1286
L1290
L1312

I can drop the others but seemed likely to cause obscure issues later. Do you want me
to drop them?

I can also drop the double-indented tests but seems to me that it would simply be buggy if it overintented the code due to header guards or if the define was placed in code.

Maybe would be cleaner if we made Level a new struct and overloading +/- to check if in PP region to keep track of PP level?

[sstwcw]

Here is one problem:

clang-format -style='{IndentPPDirectives: BeforeHash, PPIndentWidth: 1, IndentWidth: 4, IndentCaseLabels: true}'

actual:
#define X                                                                      \
 switch (x) {                                                                  \
     case 0:                                                                   \
      break;                                                                   \
     default:                                                                  \
      break;                                                                   \
 }

expected:
#define X                                                                      \
 switch (x) {                                                                  \
     case 0:                                                                   \
         break;                                                                \
     default:                                                                  \
         break;                                                                \
 }

Me guessing what caused the problem. You intend for PPLevel to mean the depth of #if sections. But you accidentally made it change along with Level where you shouldn't, for example in UnwrappedLineParser.cpp: 600.

In addition, I suggest including PPBranchLevel in PPLevel only instead of also in Level.

clang/lib/Format/UnwrappedLineParser.h

47

Please elaborate what preprocessor indent level means.

[owenpan]

In D137181#3910800, @goldstein.w.n wrote:

In D137181#3910799, @owenpan wrote:

IMO we should find a simpler way to indent bodies of macro definitions that are nested more than one level deep. I prefer we handle that in another patch and only support the examples in the summary for now.

I'm not sure what changes to the patch that would imply?

I meant only making changes to UnwrappedLineFormatter.cpp.

Also want to note, for the unit tests to all pass only 4-places actually need the
PPLevel tracking.
L373
L1286
L1290
L1312

I think you were referring to UnwrappedLineParser.cpp here. Anyway, I think most of the added tests are incorrect: the first line in a macro body should be indented 3 columns relative to define as explained in D137181#3910566.

One idea I've experimented with is to pass PPBranchLevel from UnwrappedLine to AnnotatedLine and use that in the unwrapped line formatter. It seems very promising.

[owenpan]

In D137181#3911005, @sstwcw wrote:

Here is one problem:

clang-format -style='{IndentPPDirectives: BeforeHash, PPIndentWidth: 1, IndentWidth: 4, IndentCaseLabels: true}'

actual:
#define X                                                                      \
 switch (x) {                                                                  \
     case 0:                                                                   \
      break;                                                                   \
     default:                                                                  \
      break;                                                                   \
 }

expected:
#define X                                                                      \
 switch (x) {                                                                  \
     case 0:                                                                   \
         break;                                                                \
     default:                                                                  \
         break;                                                                \
 }

Actually, the expected should be:

#define X                                                                         \
    switch (x) {                                                                  \
        case 0:                                                                   \
            break;                                                                \
        default:                                                                  \
            break;                                                                \
    }

[owenpan]

@goldstein.w.n do you need to modify MacroCallReconstructor.cpp, Macros.h, and MacroCallReconstructorTest.cpp? Leaving them out wouldn't break any existing tests.

Adding PPBranchLevel (or PPLevel in your case) to UnwrappedLine and AnnotatedLine worked for me. I suggest that you leave PPLevel alone in the annotator and don't change it at various places in the parser.

[goldstein.w.n]

In D137181#3911602, @owenpan wrote:

In D137181#3911005, @sstwcw wrote:

Here is one problem:

clang-format -style='{IndentPPDirectives: BeforeHash, PPIndentWidth: 1, IndentWidth: 4, IndentCaseLabels: true}'

actual:
#define X                                                                      \
 switch (x) {                                                                  \
     case 0:                                                                   \
      break;                                                                   \
     default:                                                                  \
      break;                                                                   \
 }

expected:
#define X                                                                      \
 switch (x) {                                                                  \
     case 0:                                                                   \
         break;                                                                \
     default:                                                                  \
         break;                                                                \
 }

Actually, the expected should be:

#define X                                                                         \
    switch (x) {                                                                  \
        case 0:                                                                   \
            break;                                                                \
        default:                                                                  \
            break;                                                                \
    }

Was able to fix this, will post new code with a test for it.

[goldstein.w.n]

In D137181#3914066, @owenpan wrote:

@goldstein.w.n do you need to modify MacroCallReconstructor.cpp, Macros.h, and MacroCallReconstructorTest.cpp? Leaving them out wouldn't break any existing tests.

Adding PPBranchLevel (or PPLevel in your case) to UnwrappedLine and AnnotatedLine worked for me. I suggest that you leave PPLevel alone in the annotator and don't change it at various places in the parser.

Hi, sorry to go mia all day. We can't drop PPLevel from MacroCallReconstructor because we need it for the createUnwrappedLine MacroCallReconstructor.cpp:L66.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Fixed case statement, remove many unnecessary PPLevel changes.

[goldstein.w.n]

In D137181#3911602, @owenpan wrote:

In D137181#3911005, @sstwcw wrote:

Here is one problem:

clang-format -style='{IndentPPDirectives: BeforeHash, PPIndentWidth: 1, IndentWidth: 4, IndentCaseLabels: true}'

actual:
#define X                                                                      \
 switch (x) {                                                                  \
     case 0:                                                                   \
      break;                                                                   \
     default:                                                                  \
      break;                                                                   \
 }

expected:
#define X                                                                      \
 switch (x) {                                                                  \
     case 0:                                                                   \
         break;                                                                \
     default:                                                                  \
         break;                                                                \
 }

Actually, the expected should be:

#define X                                                                         \
    switch (x) {                                                                  \
        case 0:                                                                   \
            break;                                                                \
        default:                                                                  \
            break;                                                                \
    }

Err I'm not sure that's right. I thought we where going for the C-code should start at the column that the 'd' in define is.
So it would be:

#define X       \
 switch (x) {   \
     case 0:    \
         break; \
     default:   \
         break; \
 }

[owenpan]

In D137181#3914083, @goldstein.w.n wrote:

In D137181#3914066, @owenpan wrote:

@goldstein.w.n do you need to modify MacroCallReconstructor.cpp, Macros.h, and MacroCallReconstructorTest.cpp? Leaving them out wouldn't break any existing tests.

Hi, sorry to go mia all day. We can't drop PPLevel from MacroCallReconstructor because we need it for the createUnwrappedLine MacroCallReconstructor.cpp:L66.

Why would you need it for createUnwrappedLine? Can you add test cases for the requirement? Removing all the changes to MacroCallReconstructor doesn't fail any existing and new tests.

[owenpan]

In D137181#3914100, @goldstein.w.n wrote:

Err I'm not sure that's right. I thought we where going for the C-code should start at the column that the 'd' in define is.
So it would be:

#define X       \
 switch (x) {   \
     case 0:    \
         break; \
     default:   \
         break; \
 }

That doesn't make sense IMO. Here is an example:

$ cat foo.cpp
#if X
#define FOO \
    if (a)  \
        b;
#endif
$ clang-format -style="{BasedOnStyle: Chromium, IndentWidth: 4}" foo.cpp > out1
$ diff foo.cpp out1
$ clang-format -style="{BasedOnStyle: Chromium, IndentWidth: 4, PPIndentWidth: 1}" foo.cpp > out2

Now what should out2 look like? Well, it should be no different than out1 (and foo.cpp) because IndentPPDirectives is still None by default. However, the current patch outputs the following instead:

#if X
#define FOO \
 if (a)     \
     b;
#endif

So I really think the first line of a macro definition body should be indented IndentWidth - 1 relative to the letter d in define no matter what IndentPPDirectives and PPIndentWidth are set to.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Remove changes to MacroCallReconstructor

[goldstein.w.n]

In D137181#3914245, @owenpan wrote:

In D137181#3914083, @goldstein.w.n wrote:

In D137181#3914066, @owenpan wrote:

@goldstein.w.n do you need to modify MacroCallReconstructor.cpp, Macros.h, and MacroCallReconstructorTest.cpp? Leaving them out wouldn't break any existing tests.

Hi, sorry to go mia all day. We can't drop PPLevel from MacroCallReconstructor because we need it for the createUnwrappedLine MacroCallReconstructor.cpp:L66.

Why would you need it for createUnwrappedLine? Can you add test cases for the requirement? Removing all the changes to MacroCallReconstructor doesn't fail any existing and new tests.

Okay fair enough. Dropped.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Make code inside macros start at common IndentWidth - 1.

[goldstein.w.n]

In D137181#3914292, @owenpan wrote:

In D137181#3914100, @goldstein.w.n wrote:

Err I'm not sure that's right. I thought we where going for the C-code should start at the column that the 'd' in define is.
So it would be:

#define X       \
 switch (x) {   \
     case 0:    \
         break; \
     default:   \
         break; \
 }

That doesn't make sense IMO. Here is an example:

$ cat foo.cpp
#if X
#define FOO \
    if (a)  \
        b;
#endif
$ clang-format -style="{BasedOnStyle: Chromium, IndentWidth: 4}" foo.cpp > out1
$ diff foo.cpp out1
$ clang-format -style="{BasedOnStyle: Chromium, IndentWidth: 4, PPIndentWidth: 1}" foo.cpp > out2

Now what should out2 look like? Well, it should be no different than out1 (and foo.cpp) because IndentPPDirectives is still None by default. However, the current patch outputs the following instead:

#if X
#define FOO \
 if (a)     \
     b;
#endif

So I really think the first line of a macro definition body should be indented IndentWidth - 1 relative to the letter d in define no matter what IndentPPDirectives and PPIndentWidth are set to.

Fair enough. Adding something like:

if (PPIndentWidth < Style.IndentWidth)
  Indent += Style.IndentWidth - PPIndentWidth;

Works for that and all existing tests pass + added a few tests with varied indentwidth / ppindentwidth.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Elaborate in comments for PPLevel

clang/lib/Format/UnwrappedLineParser.h

47

Done.

[owenpan]

You can still simply the changes to UnwrappedLineParser.cpp a lot. In fact, instead of adjusting PPLevel at various places, you only need to set it once when InMacroBody is set.

clang/lib/Format/UnwrappedLineFormatter.cpp

88–89

Please run git-clang-format.

clang/lib/Format/UnwrappedLineParser.cpp

113

Do you need to add PreviousLinePPLevel here? If yes, can you add test cases for it? Otherwise, you don't need to make any changes to ScopedMacroState.

clang/lib/Format/UnwrappedLineParser.h

66

You need to initialize PPLevel in the constructor. FWIW I'd use unsigned to be consistent with Level above.

[goldstein.w.n]

clang/lib/Format/UnwrappedLineFormatter.cpp

88–89

Will do for next submissions (thought arc did that automatically on precommit).

clang/lib/Format/UnwrappedLineParser.cpp

113

It's needed.

Without:

Expected equality of these values:
  Expected.str()
    Which is: "#ifndef foo\n#define foo\nif (emacs) {\n#ifdef is\n #define lit           \\\n     if (af) {         \\\n         return duh(); \\\n     }\n#endif\n}\n#endif"
  format(test::messUp(Code), ObjCStyle)
    Which is: "#ifndef foo\n#define foo\nif (emacs) {\n#ifdef is\n #define lit        \\\n  if (af) {         \\\n      return duh(); \\\n  }\n#endif\n}\n#endif"
With diff:
@@ -3,8 +3,8 @@
 if (emacs) {
 #ifdef is
- #define lit           \\
-     if (af) {         \\
-         return duh(); \\
-     }
+ #define lit        \\
+  if (af) {         \\
+      return duh(); \\
+  }
 #endif
 }

clang/lib/Format/UnwrappedLineParser.h

66

Will initialize. It's int because we do a less than zero check in the Formatter.

[goldstein.w.n]

In D137181#3916474, @owenpan wrote:

You can still simply the changes to UnwrappedLineParser.cpp a lot. In fact, instead of adjusting PPLevel at various places, you only need to set it once when InMacroBody is set.

I don't think thats right, it needs to stay updated with the true macro depth (even though we only use PPLevel for formatting InMacroBody lines).

[owenpan]

In D137181#3916488, @goldstein.w.n wrote:

In D137181#3916474, @owenpan wrote:

You can still simply the changes to UnwrappedLineParser.cpp a lot. In fact, instead of adjusting PPLevel at various places, you only need to set it once when InMacroBody is set.

I don't think thats right, it needs to stay updated with the true macro depth (even though we only use PPLevel for formatting InMacroBody lines).

My experiment from a few days ago was successful and also passes all the new tests you just added here. If your experience told you otherwise, then perhaps there are some missing test cases.

[goldstein.w.n]

In D137181#3916505, @owenpan wrote:

In D137181#3916488, @goldstein.w.n wrote:

In D137181#3916474, @owenpan wrote:

You can still simply the changes to UnwrappedLineParser.cpp a lot. In fact, instead of adjusting PPLevel at various places, you only need to set it once when InMacroBody is set.

I don't think thats right, it needs to stay updated with the true macro depth (even though we only use PPLevel for formatting InMacroBody lines).

I think we can remove the places I set InitialPPLevel and OldPPLevel but anything else I remove causes at least one test to fail.

My experiment from a few days ago was successful and also passes all the new tests you just added here. If your experience told you otherwise, then perhaps there are some missing test cases.

What did you do? Just set PPLevel = PPBranchLevel?

[owenpan]

In D137181#3916521, @goldstein.w.n wrote:

I think we can remove the places I set InitialPPLevel and OldPPLevel but anything else I remove causes at least one test to fail.

What did you do? Just set PPLevel = PPBranchLevel?

Yes, if there is a header guard. Otherwise, set PPLevel to PPBranchLevel + 1.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

113

I don't have to touch ScopedMacroState. Perhaps it has something to do with the way you set/adjust/use PPLevel.

[goldstein.w.n]

In D137181#3916547, @owenpan wrote:

In D137181#3916521, @goldstein.w.n wrote:

I think we can remove the places I set InitialPPLevel and OldPPLevel but anything else I remove causes at least one test to fail.

What did you do? Just set PPLevel = PPBranchLevel?

Yes, if there is a header guard. Otherwise, set PPLevel to PPBranchLevel + 1.

That fails alot of the tests for me.

I have a patch attached (applyable on this) maybe you did something different?

v1-0001-Remove-all-other-defs.patch4 KBDownload

The best I'm able to do while this passing all the tests is remove the use setting InitialPPLevel and OldPPLevel. Worth noting in the last few days I've added several new tests (for inside of a scope + case statement + header guard cases and those seem to be the ones failing so it may just be the old tests where insufficient).

[owenpan]

clang/lib/Format/UnwrappedLineParser.h

66

It's int because we do a less than zero check in the Formatter.

If you set PPLevel to PPBranchLevel when there is no header guard, and to PPBranchLevel + 1otherwise, PPLevel can never be negative.

[owenpan]

In D137181#3916558, @goldstein.w.n wrote:

In D137181#3916547, @owenpan wrote:

Yes, if there is a header guard. Otherwise, set PPLevel to PPBranchLevel + 1.

That fails alot of the tests for me.

maybe you did something different?

Here is what I did:

$ git diff UnwrappedLineParser.cpp
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 25d9018fa109..ab3b9c53ee54 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -197,6 +197,7 @@ public:
     PreBlockLine = std::move(Parser.Line);
     Parser.Line = std::make_unique<UnwrappedLine>();
     Parser.Line->Level = PreBlockLine->Level;
+    Parser.Line->PPLevel = PreBlockLine->PPLevel;
     Parser.Line->InPPDirective = PreBlockLine->InPPDirective;
     Parser.Line->InMacroBody = PreBlockLine->InMacroBody;
   }
@@ -1274,6 +1275,10 @@ void UnwrappedLineParser::parsePPDefine() {
   addUnwrappedLine();
   ++Line->Level;
   Line->InMacroBody = true;
+  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None) {
+    Line->PPLevel =
+        IncludeGuard == IG_Defined ? PPBranchLevel : PPBranchLevel + 1;
+  }
 
   // Errors during a preprocessor directive can only affect the layout of the
   // preprocessor directive, and thus we ignore them. An alternative approach

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Remove many unnecessary PPLevel changes.

[goldstein.w.n]

In D137181#3916605, @owenpan wrote:

In D137181#3916558, @goldstein.w.n wrote:

In D137181#3916547, @owenpan wrote:

Yes, if there is a header guard. Otherwise, set PPLevel to PPBranchLevel + 1.

That fails alot of the tests for me.

maybe you did something different?

Here is what I did:

$ git diff UnwrappedLineParser.cpp
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 25d9018fa109..ab3b9c53ee54 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -197,6 +197,7 @@ public:
     PreBlockLine = std::move(Parser.Line);
     Parser.Line = std::make_unique<UnwrappedLine>();
     Parser.Line->Level = PreBlockLine->Level;
+    Parser.Line->PPLevel = PreBlockLine->PPLevel;
     Parser.Line->InPPDirective = PreBlockLine->InPPDirective;
     Parser.Line->InMacroBody = PreBlockLine->InMacroBody;
   }
@@ -1274,6 +1275,10 @@ void UnwrappedLineParser::parsePPDefine() {
   addUnwrappedLine();
   ++Line->Level;
   Line->InMacroBody = true;
+  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None) {
+    Line->PPLevel =
+        IncludeGuard == IG_Defined ? PPBranchLevel : PPBranchLevel + 1;
+  }
 
   // Errors during a preprocessor directive can only affect the layout of the
   // preprocessor directive, and thus we ignore them. An alternative approach

You're right this works (but off by one). Updated patch.

[owenpan]

In D137181#3918117, @goldstein.w.n wrote:

maybe you did something different?

Here is what I did:

$ git diff UnwrappedLineParser.cpp
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 25d9018fa109..ab3b9c53ee54 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -197,6 +197,7 @@ public:
     PreBlockLine = std::move(Parser.Line);
     Parser.Line = std::make_unique<UnwrappedLine>();
     Parser.Line->Level = PreBlockLine->Level;
+    Parser.Line->PPLevel = PreBlockLine->PPLevel;
     Parser.Line->InPPDirective = PreBlockLine->InPPDirective;
     Parser.Line->InMacroBody = PreBlockLine->InMacroBody;
   }
@@ -1274,6 +1275,10 @@ void UnwrappedLineParser::parsePPDefine() {
   addUnwrappedLine();
   ++Line->Level;
   Line->InMacroBody = true;
+  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None) {
+    Line->PPLevel =
+        IncludeGuard == IG_Defined ? PPBranchLevel : PPBranchLevel + 1;
+  }
 
   // Errors during a preprocessor directive can only affect the layout of the
   // preprocessor directive, and thus we ignore them. An alternative approach

You're right this works (but off by one). Updated patch.

It was not off by 1, or else it wouldn't have worked. Below is how I defined PPLevel:

$ git diff UnwrappedLineParser.h
diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
index b9b106bcc89a..a234f6852e0c 100644
--- a/clang/lib/Format/UnwrappedLineParser.h
+++ b/clang/lib/Format/UnwrappedLineParser.h
@@ -43,6 +43,9 @@ struct UnwrappedLine {
 
   /// The indent level of the \c UnwrappedLine.
   unsigned Level;
+  /// The \c PPBranchLevel (adjusted for header guards) of the macro definition
+  /// this line belongs to.
+  unsigned PPLevel;
 
   /// Whether this \c UnwrappedLine is part of a preprocessor directive.
   bool InPPDirective;
@@ -358,7 +361,7 @@ struct UnwrappedLineNode {
 };
 
 inline UnwrappedLine::UnwrappedLine()
-    : Level(0), InPPDirective(false), InPragmaDirective(false),
+    : Level(0), PPLevel(0), InPPDirective(false), InPragmaDirective(false),
       InMacroBody(false), MustBeDeclaration(false),
       MatchingOpeningBlockLineIndex(kInvalidIndex) {}

Conceptually, I think it's more accurate to make PPLevel to mean the PP branching level of the #define line, not the first line of the macro body. IMO it may simplify the changes you made to the formatter.

BTW you still need to change PPLevel to unsigned and initialize it as requested in D137181#inline-1328385.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Make PPLevel unsigned + Initialize in constructor.

[goldstein.w.n]

In D137181#3918673, @owenpan wrote:

In D137181#3918117, @goldstein.w.n wrote:

maybe you did something different?

Here is what I did:

$ git diff UnwrappedLineParser.cpp
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 25d9018fa109..ab3b9c53ee54 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -197,6 +197,7 @@ public:
     PreBlockLine = std::move(Parser.Line);
     Parser.Line = std::make_unique<UnwrappedLine>();
     Parser.Line->Level = PreBlockLine->Level;
+    Parser.Line->PPLevel = PreBlockLine->PPLevel;
     Parser.Line->InPPDirective = PreBlockLine->InPPDirective;
     Parser.Line->InMacroBody = PreBlockLine->InMacroBody;
   }
@@ -1274,6 +1275,10 @@ void UnwrappedLineParser::parsePPDefine() {
   addUnwrappedLine();
   ++Line->Level;
   Line->InMacroBody = true;
+  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None) {
+    Line->PPLevel =
+        IncludeGuard == IG_Defined ? PPBranchLevel : PPBranchLevel + 1;
+  }
 
   // Errors during a preprocessor directive can only affect the layout of the
   // preprocessor directive, and thus we ignore them. An alternative approach

You're right this works (but off by one). Updated patch.

It was not off by 1, or else it wouldn't have worked. Below is how I defined PPLevel:

$ git diff UnwrappedLineParser.h
diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
index b9b106bcc89a..a234f6852e0c 100644
--- a/clang/lib/Format/UnwrappedLineParser.h
+++ b/clang/lib/Format/UnwrappedLineParser.h
@@ -43,6 +43,9 @@ struct UnwrappedLine {
 
   /// The indent level of the \c UnwrappedLine.
   unsigned Level;
+  /// The \c PPBranchLevel (adjusted for header guards) of the macro definition
+  /// this line belongs to.
+  unsigned PPLevel;
 
   /// Whether this \c UnwrappedLine is part of a preprocessor directive.
   bool InPPDirective;
@@ -358,7 +361,7 @@ struct UnwrappedLineNode {
 };
 
 inline UnwrappedLine::UnwrappedLine()
-    : Level(0), InPPDirective(false), InPragmaDirective(false),
+    : Level(0), PPLevel(0), InPPDirective(false), InPragmaDirective(false),
       InMacroBody(false), MustBeDeclaration(false),
       MatchingOpeningBlockLineIndex(kInvalidIndex) {}

Conceptually, I think it's more accurate to make PPLevel to mean the PP branching level of the #define line, not the first line of the macro body. IMO it may simplify the changes you made to the formatter.

Hmm? Not sure what you mean.

BTW you still need to change PPLevel to unsigned and initialize it as requested in D137181#inline-1328385.

Fixed.

clang/lib/Format/UnwrappedLineParser.h

66

Sorry didnt see your reply. Fixed

[owenpan]

In D137181#3918715, @goldstein.w.n wrote:

In D137181#3918673, @owenpan wrote:

Below is how I defined PPLevel:

$ git diff UnwrappedLineParser.h
diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
index b9b106bcc89a..a234f6852e0c 100644
--- a/clang/lib/Format/UnwrappedLineParser.h
+++ b/clang/lib/Format/UnwrappedLineParser.h
@@ -43,6 +43,9 @@ struct UnwrappedLine {
 
   /// The indent level of the \c UnwrappedLine.
   unsigned Level;
+  /// The \c PPBranchLevel (adjusted for header guards) of the macro definition
+  /// this line belongs to.
+  unsigned PPLevel;
 
   /// Whether this \c UnwrappedLine is part of a preprocessor directive.
   bool InPPDirective;
@@ -358,7 +361,7 @@ struct UnwrappedLineNode {
 };
 
 inline UnwrappedLine::UnwrappedLine()
-    : Level(0), InPPDirective(false), InPragmaDirective(false),
+    : Level(0), PPLevel(0), InPPDirective(false), InPragmaDirective(false),
       InMacroBody(false), MustBeDeclaration(false),
       MatchingOpeningBlockLineIndex(kInvalidIndex) {}

Conceptually, I think it's more accurate to make PPLevel to mean the PP branching level of the #define line, not the first line of the macro body. IMO it may simplify the changes you made to the formatter.

Hmm? Not sure what you mean.

Does the comments above unsigned PPLevel; in the above git diff output help? Anyway, you already addressed my comment by decreasing PPLevel by 1, so it's ok now.

[goldstein.w.n]

In D137181#3923434, @owenpan wrote:

In D137181#3918715, @goldstein.w.n wrote:

In D137181#3918673, @owenpan wrote:

Below is how I defined PPLevel:

$ git diff UnwrappedLineParser.h
diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
index b9b106bcc89a..a234f6852e0c 100644
--- a/clang/lib/Format/UnwrappedLineParser.h
+++ b/clang/lib/Format/UnwrappedLineParser.h
@@ -43,6 +43,9 @@ struct UnwrappedLine {
 
   /// The indent level of the \c UnwrappedLine.
   unsigned Level;
+  /// The \c PPBranchLevel (adjusted for header guards) of the macro definition
+  /// this line belongs to.
+  unsigned PPLevel;
 
   /// Whether this \c UnwrappedLine is part of a preprocessor directive.
   bool InPPDirective;
@@ -358,7 +361,7 @@ struct UnwrappedLineNode {
 };
 
 inline UnwrappedLine::UnwrappedLine()
-    : Level(0), InPPDirective(false), InPragmaDirective(false),
+    : Level(0), PPLevel(0), InPPDirective(false), InPragmaDirective(false),
       InMacroBody(false), MustBeDeclaration(false),
       MatchingOpeningBlockLineIndex(kInvalidIndex) {}

Conceptually, I think it's more accurate to make PPLevel to mean the PP branching level of the #define line, not the first line of the macro body. IMO it may simplify the changes you made to the formatter.

Hmm? Not sure what you mean.

Does the comments above unsigned PPLevel; in the above git diff output help? Anyway, you already addressed my comment by decreasing PPLevel by 1, so it's ok now.

Got it. Is the patch okay?

[owenpan]

I think we are close to the finishing line. Can you revisit the change to the formatter and clean it up? For example, casting PPLevel to unsigned is redundant now. IMO you can simply the change too.

clang/lib/Format/UnwrappedLineFormatter.cpp

63–67

You don't need to add this condition.

[sstwcw]

Can you make TokenAnnotator::printDebugInfo print PPLevel?

Since you changed the rules for indentation in UnwrappedLineFormatter, do you also need to change UnwrappedLineParser::mightFitOnOneLine?

clang/lib/Format/UnwrappedLineFormatter.cpp

69–73

What is this test for?

clang-format -style='{IndentPPDirective: BeforeHash, PPIndentWidth: 2, IndentWidth: 4, ColumnLimit: 40}'

actual:
#define X                              \
  {                                    \
    x;                                 \
    x;                                 \
  }

expected:
#define X                              \
    {                                  \
        x;                             \
        x;                             \
    }

73

What is this for?

clang-format -style='{IndentPPDirective: BeforeHash, PPIndentWidth: 4, IndentWidth: 1, ColumnLimit: 40}'

actual:
#if X
    #define X                          \
        {                              \
         x;                            \
         x;                            \
        }
#endif

expected:
#if X
    #define X                          \
     {                                 \
      x;                               \
      x;                               \
     }
#endif

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Cleanup logic in Formatter

[goldstein.w.n]

In D137181#3923921, @owenpan wrote:

I think we are close to the finishing line. Can you revisit the change to the formatter and clean it up? For example, casting PPLevel to unsigned is redundant now. IMO you can simply the change too.

Cleaned up the logic a bit.

[goldstein.w.n]

clang/lib/Format/UnwrappedLineFormatter.cpp

69–73

You're right, unneeded. Fixed and added test for it.

73

You're right, unneeded. Fixed and added test for it.

[owenpan]

In D137181#3924002, @sstwcw wrote:

Can you make TokenAnnotator::printDebugInfo print PPLevel?

@goldstein.w.n can you add it as follows?

$ git diff TokenAnnotator.cpp
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 75570552146c..536472e9d136 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5093,8 +5093,9 @@ bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
 }
 
 void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) const {
-  llvm::errs() << "AnnotatedTokens(L=" << Line.Level << ", T=" << Line.Type
-               << ", C=" << Line.IsContinuation << "):\n";
+  llvm::errs() << "AnnotatedTokens(L=" << Line.Level << ", P=" << Line.PPLevel
+               << ", T=" << Line.Type << ", C=" << Line.IsContinuation
+               << "):\n";
   const FormatToken *Tok = Line.First;
   while (Tok) {
     llvm::errs() << " M=" << Tok->MustBreakBefore

Since you changed the rules for indentation in UnwrappedLineFormatter, do you also need to change UnwrappedLineParser::mightFitOnOneLine?

mightFitOnOneLine is not called on PPDirective lines, but it's a good idea to add an assertion.

clang/lib/Format/UnwrappedLineParser.cpp

836–839

We don't call mightFitOnOneLine on PPDirective lines.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Add PPLevel to debug printout + assert not in macro in mightFitOnOneLine

[goldstein.w.n]

In D137181#3926848, @owenpan wrote:

In D137181#3924002, @sstwcw wrote:

Can you make TokenAnnotator::printDebugInfo print PPLevel?

@goldstein.w.n can you add it as follows?

$ git diff TokenAnnotator.cpp
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 75570552146c..536472e9d136 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5093,8 +5093,9 @@ bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
 }
 
 void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) const {
-  llvm::errs() << "AnnotatedTokens(L=" << Line.Level << ", T=" << Line.Type
-               << ", C=" << Line.IsContinuation << "):\n";
+  llvm::errs() << "AnnotatedTokens(L=" << Line.Level << ", P=" << Line.PPLevel
+               << ", T=" << Line.Type << ", C=" << Line.IsContinuation
+               << "):\n";
   const FormatToken *Tok = Line.First;
   while (Tok) {
     llvm::errs() << " M=" << Tok->MustBreakBefore

Done.

Since you changed the rules for indentation in UnwrappedLineFormatter, do you also need to change UnwrappedLineParser::mightFitOnOneLine?

mightFitOnOneLine is not called on PPDirective lines, but it's a good idea to add an assertion.

Added assert.

[owenpan]

Please mark review comments as done if you have addressed them. Can you also clean up the test cases, removing overlapping/redundant ones, making sure a test case doesn't end with a newline (e.g., line 5380), etc?

clang/lib/Format/UnwrappedLineFormatter.cpp

63–67

Please ignore my comment above.

69–74

To make it simpler and clearer.

clang/lib/Format/UnwrappedLineParser.cpp

1278

We don't need the if statement anymore. Actually, it's better to always set PPLevel for InMacroBody lines.

clang/lib/Format/UnwrappedLineParser.h

47–65

We probably don't need the (outdated) examples as we can print PPLevel in TokenAnnotator::printDebugInfo now.

clang/unittests/Format/FormatTest.cpp

5401–5403

Please delete.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

838

Shouldn't it be assert(!Line.InMacroBody && !InPPDirective) instead? You can also assert Line.PPLevel == 0.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Fix some nits.

clang/lib/Format/UnwrappedLineFormatter.cpp

69–74

Done.

clang/lib/Format/UnwrappedLineParser.cpp

838

I guess the goal was to assert condition where PPLevel would be used to calculate if a line might find (if both InMacroBody and InPPDirective) are true. But your assert should also works fine so fixed.

1278

Fixed.

clang/unittests/Format/FormatTest.cpp

5401–5403

Fixed.

[goldstein.w.n]

In D137181#3935752, @owenpan wrote:

Please mark review comments as done if you have addressed them. Can you also clean up the test cases, removing overlapping/redundant ones, making sure a test case doesn't end with a newline (e.g., line 5380), etc?

Regarding newlines. I have new line before changing the style. Otherwise done. (let me know if you want me to remove those).

Regarding removing redundant test cases. I think I've had each of them fail independently at some point (although many when I was doing the previous PPLevel tracking) so I wouldn't call any of them truly redundant.
I could remove either the PPDIS_BeforeHash or PPDIS_AfterHash though as the they really no independent logic in this commit.

Thoughts?

[owenpan]

In D137181#3935856, @goldstein.w.n wrote:

In D137181#3935752, @owenpan wrote:

Please mark review comments as done if you have addressed them. Can you also clean up the test cases, removing overlapping/redundant ones, making sure a test case doesn't end with a newline (e.g., line 5380), etc?

Regarding newlines. I have new line before changing the style. Otherwise done. (let me know if you want me to remove those).

Sorry, it's line 5379 and several other places that have an ending newline. (Just search for \n",.) You only need to remove them in the new test cases. We can fix the existing ones in a separate NFC patch.

Regarding removing redundant test cases. I think I've had each of them fail independently at some point (although many when I was doing the previous PPLevel tracking) so I wouldn't call any of them truly redundant.

I know, but IMO some of the failures (e.g. the switch statement) during development shouldn't automatically go in. This file already has a humongous size. Maybe do the best you can here.

I could remove either the PPDIS_BeforeHash or PPDIS_AfterHash though as the they really no independent logic in this commit.

Yes, please. Or better yet, alternate a little bit between the two.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Cleanup tests file

[goldstein.w.n]

In D137181#3935951, @owenpan wrote:

In D137181#3935856, @goldstein.w.n wrote:

In D137181#3935752, @owenpan wrote:

Please mark review comments as done if you have addressed them. Can you also clean up the test cases, removing overlapping/redundant ones, making sure a test case doesn't end with a newline (e.g., line 5380), etc?

Regarding newlines. I have new line before changing the style. Otherwise done. (let me know if you want me to remove those).

Sorry, it's line 5379 and several other places that have an ending newline. (Just search for \n",.) You only need to remove them in the new test cases. We can fix the existing ones in a separate NFC patch.

Done. Sorry I had thought you mean the newline in the file between the test cases.

Regarding removing redundant test cases. I think I've had each of them fail independently at some point (although many when I was doing the previous PPLevel tracking) so I wouldn't call any of them truly redundant.

I know, but IMO some of the failures (e.g. the switch statement) during development shouldn't automatically go in. This file already has a humongous size. Maybe do the best you can here.

I could remove either the PPDIS_BeforeHash or PPDIS_AfterHash though as the they really no independent logic in this commit.

Yes, please. Or better yet, alternate a little bit between the two.

Done.

[owenpan]

In D137181#3935951, @owenpan wrote:

In D137181#3935856, @goldstein.w.n wrote:

I could remove either the PPDIS_BeforeHash or PPDIS_AfterHash though as the they really no independent logic in this commit.

Yes, please. Or better yet, alternate a little bit between the two.

My bad. I meant splitting the test cases between the two. Can you regroup them (lines 5137-5258) as follows?

style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
style.IndentWidth = 4;
style.PPIndentWidth = 1;
verifyFormat("#ifdef foo\n"
             "# define bar() \\\n"
             "     if (A) {  \\\n"
             "         B();  \\\n"
             "     }         \\\n"
             "     C();\n"
             "#endif",
             style);
verifyFormat("#if abc\n"
             "# ifdef foo\n"
             "#  define bar()    \\\n"
             "      if (A) {     \\\n"
             "          if (B) { \\\n"
             "              C(); \\\n"
             "          }        \\\n"
             "      }            \\\n"
             "      D();\n"
             "# endif\n"
             "#endif",
             style);
verifyFormat("#ifndef foo\n"
             "#define foo\n"
             "if (emacs) {\n"
             "#ifdef is\n"
             "# define lit           \\\n"
             "     if (af) {         \\\n"
             "         return duh(); \\\n"
             "     }\n"
             "#endif\n"
             "}\n"
             "#endif",
             style);
verifyFormat("#define X  \\\n"
             "    {      \\\n"
             "        x; \\\n"
             "        x; \\\n"
             "    }",
             style);

style.IndentWidth = 8;
style.PPIndentWidth = 2;
verifyFormat("#ifdef foo\n"
             "#  define bar()        \\\n"
             "          if (A) {     \\\n"
             "                  B(); \\\n"
             "          }            \\\n"
             "          C();\n"
             "#endif",
             style);

style.IndentWidth = 1;
style.PPIndentWidth = 4;
verifyFormat("#define X \\\n"
             " {        \\\n"
             "  x;      \\\n"
             "  x;      \\\n"
             " }",
             style);

style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
style.IndentWidth = 4;
style.PPIndentWidth = 1;
verifyFormat("if (emacs) {\n"
             "#ifdef is\n"
             " #define lit           \\\n"
             "     if (af) {         \\\n"
             "         return duh(); \\\n"
             "     }\n"
             "#endif\n"
             "}",
             style);
verifyFormat("#if abc\n"
             " #ifdef foo\n"
             "  #define bar() \\\n"
             "      if (A) {  \\\n"
             "          B();  \\\n"
             "      }         \\\n"
             "      C();\n"
             " #endif\n"
             "#endif",
             style);
verifyFormat("#if 1\n"
             " #define X  \\\n"
             "     {      \\\n"
             "         x; \\\n"
             "         x; \\\n"
             "     }\n"
             "#endif",
             style);

style.PPIndentWidth = 2;
verifyFormat("#ifdef foo\n"
             "  #define bar() \\\n"
             "      if (A) {  \\\n"
             "          B();  \\\n"
             "      }         \\\n"
             "      C();\n"
             "#endif",
             style);

style.IndentWidth = 1;
style.PPIndentWidth = 4;
verifyFormat("#if 1\n"
             "    #define X \\\n"
             "     {        \\\n"
             "      x;      \\\n"
             "      x;      \\\n"
             "     }\n"
             "#endif",
             style);

clang/lib/Format/UnwrappedLineParser.cpp

838

error: use of undeclared identifier 'InPPDirective'
  assert(!Line.InMacroBody && !InPPDirective);
                               ^

Also, can you break it into two assertions as suggested so that we know which one failed even without a debugger?

1280

In case PPLevel is negative.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

De-interleave test cases.
Cleanup asserts.

[goldstein.w.n]

In D137181#3938771, @owenpan wrote:

In D137181#3935951, @owenpan wrote:

In D137181#3935856, @goldstein.w.n wrote:

I could remove either the PPDIS_BeforeHash or PPDIS_AfterHash though as the they really no independent logic in this commit.

Yes, please. Or better yet, alternate a little bit between the two.

My bad. I meant splitting the test cases between the two. Can you regroup them (lines 5137-5258) as follows?

style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
style.IndentWidth = 4;
style.PPIndentWidth = 1;
verifyFormat("#ifdef foo\n"
             "# define bar() \\\n"
             "     if (A) {  \\\n"
             "         B();  \\\n"
             "     }         \\\n"
             "     C();\n"
             "#endif",
             style);
verifyFormat("#if abc\n"
             "# ifdef foo\n"
             "#  define bar()    \\\n"
             "      if (A) {     \\\n"
             "          if (B) { \\\n"
             "              C(); \\\n"
             "          }        \\\n"
             "      }            \\\n"
             "      D();\n"
             "# endif\n"
             "#endif",
             style);
verifyFormat("#ifndef foo\n"
             "#define foo\n"
             "if (emacs) {\n"
             "#ifdef is\n"
             "# define lit           \\\n"
             "     if (af) {         \\\n"
             "         return duh(); \\\n"
             "     }\n"
             "#endif\n"
             "}\n"
             "#endif",
             style);
verifyFormat("#define X  \\\n"
             "    {      \\\n"
             "        x; \\\n"
             "        x; \\\n"
             "    }",
             style);

style.IndentWidth = 8;
style.PPIndentWidth = 2;
verifyFormat("#ifdef foo\n"
             "#  define bar()        \\\n"
             "          if (A) {     \\\n"
             "                  B(); \\\n"
             "          }            \\\n"
             "          C();\n"
             "#endif",
             style);

style.IndentWidth = 1;
style.PPIndentWidth = 4;
verifyFormat("#define X \\\n"
             " {        \\\n"
             "  x;      \\\n"
             "  x;      \\\n"
             " }",
             style);

style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
style.IndentWidth = 4;
style.PPIndentWidth = 1;
verifyFormat("if (emacs) {\n"
             "#ifdef is\n"
             " #define lit           \\\n"
             "     if (af) {         \\\n"
             "         return duh(); \\\n"
             "     }\n"
             "#endif\n"
             "}",
             style);
verifyFormat("#if abc\n"
             " #ifdef foo\n"
             "  #define bar() \\\n"
             "      if (A) {  \\\n"
             "          B();  \\\n"
             "      }         \\\n"
             "      C();\n"
             " #endif\n"
             "#endif",
             style);
verifyFormat("#if 1\n"
             " #define X  \\\n"
             "     {      \\\n"
             "         x; \\\n"
             "         x; \\\n"
             "     }\n"
             "#endif",
             style);

style.PPIndentWidth = 2;
verifyFormat("#ifdef foo\n"
             "  #define bar() \\\n"
             "      if (A) {  \\\n"
             "          B();  \\\n"
             "      }         \\\n"
             "      C();\n"
             "#endif",
             style);

style.IndentWidth = 1;
style.PPIndentWidth = 4;
verifyFormat("#if 1\n"
             "    #define X \\\n"
             "     {        \\\n"
             "      x;      \\\n"
             "      x;      \\\n"
             "     }\n"
             "#endif",
             style);

Yes done.

clang/lib/Format/UnwrappedLineParser.cpp

1280

Done.

[goldstein.w.n]

clang/lib/Format/UnwrappedLineParser.cpp

838

Fixed. Sorry. Had only been rebuilting formattests. Figured everything else must have a dep.

[owenpan]

LGTM

[goldstein.w.n]

In D137181#3938904, @owenpan wrote:

LGTM

Awesome! Thanks for sticking with it through all these revisions :)

Anything left for me todo (can't imagine I have push access).

[owenpan]

In D137181#3938913, @goldstein.w.n wrote:

Anything left for me todo (can't imagine I have push access).

See here. We will need your name and email.

[goldstein.w.n]

1. Updating D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Add tag to commit message

[goldstein.w.n]

In D137181#3938942, @owenpan wrote:

In D137181#3938913, @goldstein.w.n wrote:

Anything left for me todo (can't imagine I have push access).

See here. We will need your name and email.

Added [clang-format] tag to commit message.

Name: Noah Goldstein
email: [e-mail]

but that is already in the commit info.

clang/lib/Format/TokenAnnotator.h

enum LineType {
   LT_ArrayOfStructInitializer,
   LT_CommentAbovePPDirective,
 };
 
 class AnnotatedLine {
 public:
   AnnotatedLine(const UnwrappedLine &Line)
       : First(Line.Tokens.front().Tok), Level(Line.Level),
+        PPLevel(Line.PPLevel),
         MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex),
         MatchingClosingBlockLineIndex(Line.MatchingClosingBlockLineIndex),
         InPPDirective(Line.InPPDirective),
         InPragmaDirective(Line.InPragmaDirective),
         InMacroBody(Line.InMacroBody),
         MustBeDeclaration(Line.MustBeDeclaration), MightBeFunctionDecl(false),
         IsMultiVariableDeclStmt(false), Affected(false),
         LeadingEmptyLinesAffected(false), ChildrenAffected(false),
public:
 
   FormatToken *First;
   FormatToken *Last;
 
   SmallVector<AnnotatedLine *, 0> Children;
 
   LineType Type;
   unsigned Level;
+  unsigned PPLevel;
   size_t MatchingOpeningBlockLineIndex;
   size_t MatchingClosingBlockLineIndex;
   bool InPPDirective;
   bool InPragmaDirective;
   bool InMacroBody;
   bool MustBeDeclaration;
   bool MightBeFunctionDecl;
   bool IsMultiVariableDeclStmt;

clang/lib/Format/TokenAnnotator.cpp

return Left.isOneOf(tok::comma, tok::coloncolon, tok::semi, tok::l_brace,
                        tok::colon, tok::l_square, tok::at) ||
          (Left.is(tok::r_paren) &&
           Right.isOneOf(tok::identifier, tok::kw_const)) ||
          (Left.is(tok::l_paren) && !Right.is(tok::r_paren)) ||
          (Left.is(TT_TemplateOpener) && !Right.is(TT_TemplateCloser));
 }
 
 void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) const {
-  llvm::errs() << "AnnotatedTokens(L=" << Line.Level << ", T=" << Line.Type
-               << ", C=" << Line.IsContinuation << "):\n";
+  llvm::errs() << "AnnotatedTokens(L=" << Line.Level << ", P=" << Line.PPLevel
+               << ", T=" << Line.Type << ", C=" << Line.IsContinuation
+               << "):\n";
   const FormatToken *Tok = Line.First;
   while (Tok) {
     llvm::errs() << " M=" << Tok->MustBreakBefore
                  << " C=" << Tok->CanBreakBefore
                  << " T=" << getTokenTypeName(Tok->getType())
                  << " S=" << Tok->SpacesRequiredBefore
                  << " F=" << Tok->Finalized << " B=" << Tok->BlockParameterCount
                  << " BK=" << Tok->getBlockKind() << " P=" << Tok->SplitPenalty

clang/lib/Format/UnwrappedLineFormatter.cpp

public:
 
   /// Update the indent state given that \p Line is going to be formatted
   /// next.
   void nextLine(const AnnotatedLine &Line) {
     Offset = getIndentOffset(*Line.First);
     // Update the indent level cache size so that we can rely on it
     // having the right size in adjustToUnmodifiedline.
     skipLine(Line, /*UnknownIndent=*/true);
-    if (Line.InPPDirective ||
+    if (Style.IndentPPDirectives != FormatStyle::PPDIS_None &&
+        (Line.InPPDirective ||
          (Style.IndentPPDirectives == FormatStyle::PPDIS_BeforeHash &&
-         Line.Type == LT_CommentAbovePPDirective)) {
-      unsigned IndentWidth =
+          Line.Type == LT_CommentAbovePPDirective))) {
+      unsigned PPIndentWidth =
           (Style.PPIndentWidth >= 0) ? Style.PPIndentWidth : Style.IndentWidth;
-      Indent = Line.Level * IndentWidth + AdditionalIndent;
+      Indent = Line.InMacroBody
+                   ? Line.PPLevel * PPIndentWidth +
+                         (Line.Level - Line.PPLevel) * Style.IndentWidth
+                   : Line.Level * PPIndentWidth;
+      Indent += AdditionalIndent;
     } else {
       Indent = getIndent(Line.Level);
     }
     if (static_cast<int>(Indent) + Offset >= 0)
       Indent += Offset;
     if (Line.IsContinuation)
       Indent = Line.Level * Style.IndentWidth + Style.ContinuationIndentWidth;
   }
 
   /// Update the indent state given that \p Line indent should be
   /// skipped.
   void skipLine(const AnnotatedLine &Line, bool UnknownIndent = false) {
     if (Line.Level >= IndentForLevel.size())
       IndentForLevel.resize(Line.Level + 1, UnknownIndent ? -1 : Indent);
   }
 
   /// Update the level indent to adapt to the given \p Line.
   ///
   /// When a line is not formatted, we move the subsequent lines on the same
   /// level to the same indent.
   /// Note that \c nextLine must have been called before this method.
   void adjustToUnmodifiedLine(const AnnotatedLine &Line) {
     unsigned LevelIndent = Line.First->OriginalColumn;
     if (static_cast<int>(LevelIndent) - Offset >= 0)

clang/lib/Format/UnwrappedLineParser.h

struct UnwrappedLine {
   UnwrappedLine();
 
   /// The \c Tokens comprising this \c UnwrappedLine.
   std::list<UnwrappedLineNode> Tokens;
 
   /// The indent level of the \c UnwrappedLine.
   unsigned Level;
 
+  /// The \c PPBranchLevel (adjusted for header guards) if this line is a
+  /// \c InMacroBody line, and 0 otherwise.
+  unsigned PPLevel;
+
   /// Whether this \c UnwrappedLine is part of a preprocessor directive.
   bool InPPDirective;
   /// Whether this \c UnwrappedLine is part of a pramga directive.
   bool InPragmaDirective;
   /// Whether it is part of a macro body.
   bool InMacroBody;
 
   bool MustBeDeclaration;
 
   /// \c True if this line should be indented by ContinuationIndent in
   /// addition to the normal indention level.
   bool IsContinuation = false;
 
   /// If this \c UnwrappedLine closes a block in a sequence of lines,
   /// \c MatchingOpeningBlockLineIndex stores the index of the corresponding
   /// opening line. Otherwise, \c MatchingOpeningBlockLineIndex must be
   /// \c kInvalidIndex.
   size_t MatchingOpeningBlockLineIndex = kInvalidIndex;
 
   /// If this \c UnwrappedLine opens a block, stores the index of the
   /// line with the corresponding closing brace.
   size_t MatchingClosingBlockLineIndex = kInvalidIndex;
 
   static const size_t kInvalidIndex = -1;
struct UnwrappedLineNode {
   UnwrappedLineNode() : Tok(nullptr) {}
   UnwrappedLineNode(FormatToken *Tok) : Tok(Tok) {}
 
   FormatToken *Tok;
   SmallVector<UnwrappedLine, 0> Children;
 };
 
 inline UnwrappedLine::UnwrappedLine()
-    : Level(0), InPPDirective(false), InPragmaDirective(false),
+    : Level(0), PPLevel(0), InPPDirective(false), InPragmaDirective(false),
       InMacroBody(false), MustBeDeclaration(false),
       MatchingOpeningBlockLineIndex(kInvalidIndex) {}
 
 } // end namespace format
 } // end namespace clang
 
 #endif

clang/lib/Format/UnwrappedLineParser.cpp

 }
 
 class ScopedMacroState : public FormatTokenSource {
 public:
   ScopedMacroState(UnwrappedLine &Line, FormatTokenSource *&TokenSource,
                    FormatToken *&ResetToken)
       : Line(Line), TokenSource(TokenSource), ResetToken(ResetToken),
         PreviousLineLevel(Line.Level), PreviousTokenSource(TokenSource),
         Token(nullptr), PreviousToken(nullptr) {
     FakeEOF.Tok.startToken();
     FakeEOF.Tok.setKind(tok::eof);
     TokenSource = this;
     Line.Level = 0;
     Line.InPPDirective = true;
     // InMacroBody gets set after the `#define x` part.
   }
 
ScopedLineState(UnwrappedLineParser &Parser,
       : Parser(Parser), OriginalLines(Parser.CurrentLines) {
     if (SwitchToPreprocessorLines)
       Parser.CurrentLines = &Parser.PreprocessorDirectives;
     else if (!Parser.Line->Tokens.empty())
       Parser.CurrentLines = &Parser.Line->Tokens.back().Children;
     PreBlockLine = std::move(Parser.Line);
     Parser.Line = std::make_unique<UnwrappedLine>();
     Parser.Line->Level = PreBlockLine->Level;
+    Parser.Line->PPLevel = PreBlockLine->PPLevel;
     Parser.Line->InPPDirective = PreBlockLine->InPPDirective;
     Parser.Line->InMacroBody = PreBlockLine->InMacroBody;
   }
 
   ~ScopedLineState() {
     if (!Parser.Line->Tokens.empty())
       Parser.addUnwrappedLine();
     assert(Parser.Line->Tokens.empty());
bool UnwrappedLineParser::mightFitOnOneLine(
 
   Index = 0;
   for (auto &Token : Tokens) {
     const auto &SavedToken = SavedTokens[Index++];
     Token.Tok->copyFrom(*SavedToken.Tok);
     Token.Children = std::move(SavedToken.Children);
     delete SavedToken.Tok;
   }
 
+  // If these change PPLevel needs to be used for get correct indentation.
+  assert(!Line.InMacroBody);
+  assert(!Line.InPPDirective);
   return Line.Level * Style.IndentWidth + Length <= ColumnLimit;
 }
 
 FormatToken *UnwrappedLineParser::parseBlock(
     bool MustBeDeclaration, unsigned AddLevels, bool MunchSemi, bool KeepBraces,
     IfStmtKind *IfKind, bool UnindentWhitesmithsBraces,
     bool CanContainBracedList, TokenType NextLBracesType) {
   auto HandleVerilogBlockLabel = [this]() {
void UnwrappedLineParser::parsePPDefine() {
   if (FormatTok->Tok.getKind() == tok::l_paren &&
       !FormatTok->hasWhitespaceBefore()) {
     parseParens();
   }
   if (Style.IndentPPDirectives != FormatStyle::PPDIS_None)
     Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
   ++Line->Level;
+
+  Line->PPLevel = PPBranchLevel + (IncludeGuard == IG_Defined ? 0 : 1);
+  assert((int)Line->PPLevel >= 0);
   Line->InMacroBody = true;
 
   // Errors during a preprocessor directive can only affect the layout of the
   // preprocessor directive, and thus we ignore them. An alternative approach
   // would be to use the same approach we use on the file level (no
   // re-indentation if there was a structural error) within the macro
   // definition.
   parseFile();
 }

clang/unittests/Format/FormatTest.cpp

verifyFormat("#ifdef __linux__\n"
                "    int x = 0;\n"
                "}\n"
                " #define FOO foo\n"
                "#endif\n"
                "void bar() {\n"
                "    int y = 0;\n"
                "}\n",
                style);
   verifyFormat("#if 1\n"
                " // some comments\n"
                " // another\n"
                " #define foo 1\n"
                "// not a define comment\n"
                "void bar() {\n"
                "    // comment\n"
                "    int y = 0;\n"
                "}",
                "#if 1\n"
                "// some comments\n"
                "// another\n"
                "#define foo 1\n"
                "// not a define comment\n"
                "void bar() {\n"
                "  // comment\n"
                "  int y = 0;\n"
                "}",
                style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef foo\n"
+               "#define bar() \\\n"
+               "    if (A) {  \\\n"
+               "        B();  \\\n"
+               "    }         \\\n"
+               "    C();\n"
+               "#endif",
+               style);
+  verifyFormat("if (emacs) {\n"
+               "#ifdef is\n"
+               "#define lit           \\\n"
+               "    if (af) {         \\\n"
+               "        return duh(); \\\n"
+               "    }\n"
+               "#endif\n"
+               "}",
+               style);
+  verifyFormat("#if abc\n"
+               "#ifdef foo\n"
+               "#define bar()    \\\n"
+               "    if (A) {     \\\n"
+               "        if (B) { \\\n"
+               "            C(); \\\n"
+               "        }        \\\n"
+               "    }            \\\n"
+               "    D();\n"
+               "#endif\n"
+               "#endif",
+               style);
+  verifyFormat("#ifndef foo\n"
+               "#define foo\n"
+               "if (emacs) {\n"
+               "#ifdef is\n"
+               "#define lit           \\\n"
+               "    if (af) {         \\\n"
+               "        return duh(); \\\n"
+               "    }\n"
+               "#endif\n"
+               "}\n"
+               "#endif",
+               style);
+  verifyFormat("#if 1\n"
+               "#define X  \\\n"
+               "    {      \\\n"
+               "        x; \\\n"
+               "        x; \\\n"
+               "    }\n"
+               "#endif",
+               style);
+  verifyFormat("#define X  \\\n"
+               "    {      \\\n"
+               "        x; \\\n"
+               "        x; \\\n"
+               "    }",
+               style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+               "#define bar() \\\n"
+               "    if (A) {  \\\n"
+               "        B();  \\\n"
+               "    }         \\\n"
+               "    C();\n"
+               "#endif",
+               style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+               "#define bar()        \\\n"
+               "        if (A) {     \\\n"
+               "                B(); \\\n"
+               "        }            \\\n"
+               "        C();\n"
+               "#endif",
+               style);
+
+  style.IndentWidth = 1;
+  style.PPIndentWidth = 4;
+  verifyFormat("#if 1\n"
+               "#define X \\\n"
+               " {        \\\n"
+               "  x;      \\\n"
+               "  x;      \\\n"
+               " }\n"
+               "#endif",
+               style);
+  verifyFormat("#define X \\\n"
+               " {        \\\n"
+               "  x;      \\\n"
+               "  x;      \\\n"
+               " }",
+               style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef foo\n"
+               "# define bar() \\\n"
+               "     if (A) {  \\\n"
+               "         B();  \\\n"
+               "     }         \\\n"
+               "     C();\n"
+               "#endif",
+               style);
+  verifyFormat("#if abc\n"
+               "# ifdef foo\n"
+               "#  define bar()    \\\n"
+               "      if (A) {     \\\n"
+               "          if (B) { \\\n"
+               "              C(); \\\n"
+               "          }        \\\n"
+               "      }            \\\n"
+               "      D();\n"
+               "# endif\n"
+               "#endif",
+               style);
+  verifyFormat("#ifndef foo\n"
+               "#define foo\n"
+               "if (emacs) {\n"
+               "#ifdef is\n"
+               "# define lit           \\\n"
+               "     if (af) {         \\\n"
+               "         return duh(); \\\n"
+               "     }\n"
+               "#endif\n"
+               "}\n"
+               "#endif",
+               style);
+  verifyFormat("#define X  \\\n"
+               "    {      \\\n"
+               "        x; \\\n"
+               "        x; \\\n"
+               "    }",
+               style);
+
+  style.PPIndentWidth = 2;
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+               "#  define bar()        \\\n"
+               "          if (A) {     \\\n"
+               "                  B(); \\\n"
+               "          }            \\\n"
+               "          C();\n"
+               "#endif",
+               style);
+
+  style.PPIndentWidth = 4;
+  style.IndentWidth = 1;
+  verifyFormat("#define X \\\n"
+               " {        \\\n"
+               "  x;      \\\n"
+               "  x;      \\\n"
+               " }",
+               style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+  style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
+  verifyFormat("if (emacs) {\n"
+               "#ifdef is\n"
+               " #define lit           \\\n"
+               "     if (af) {         \\\n"
+               "         return duh(); \\\n"
+               "     }\n"
+               "#endif\n"
+               "}",
+               style);
+  verifyFormat("#if abc\n"
+               " #ifdef foo\n"
+               "  #define bar() \\\n"
+               "      if (A) {  \\\n"
+               "          B();  \\\n"
+               "      }         \\\n"
+               "      C();\n"
+               " #endif\n"
+               "#endif",
+               style);
+  verifyFormat("#if 1\n"
+               " #define X  \\\n"
+               "     {      \\\n"
+               "         x; \\\n"
+               "         x; \\\n"
+               "     }\n"
+               "#endif",
+               style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+               "  #define bar() \\\n"
+               "      if (A) {  \\\n"
+               "          B();  \\\n"
+               "      }         \\\n"
+               "      C();\n"
+               "#endif",
+               style);
+
+  style.PPIndentWidth = 4;
+  style.IndentWidth = 1;
+  verifyFormat("#if 1\n"
+               "    #define X \\\n"
+               "     {        \\\n"
+               "      x;      \\\n"
+               "      x;      \\\n"
+               "     }\n"
+               "#endif",
+               style);
 }
 
 TEST_F(FormatTest, IndentsPPDirectiveInReducedSpace) {
   verifyFormat("#define A(BB)", getLLVMStyleWithColumns(13));
   verifyFormat("#define A( \\\n    BB)", getLLVMStyleWithColumns(12));
   verifyFormat("#define A( \\\n    A, B)", getLLVMStyleWithColumns(12));
   // FIXME: We never break before the macro name.
   verifyFormat("#define AA( \\\n    B)", getLLVMStyleWithColumns(12));
verifyFormat("#define MACRO(a) \\\n"
                "    f();         \\\n"
                "  else           \\\n"
                "    g()",
                getLLVMStyleWithColumns(18));
   verifyFormat("#define A template <typename T>");
   verifyIncompleteFormat("#define STR(x) #x\n"
                          "f(STR(this_is_a_string_literal{));");
   verifyFormat("#pragma omp threadprivate( \\\n"
                "        y)), // expected-warning",
                getLLVMStyleWithColumns(28));
   verifyFormat("#d, = };");
   verifyFormat("#if \"a");
   verifyIncompleteFormat("({\n"
                          "#define b     \\\n"
                          "  }           \\\n"
                          "  a\n"
                          "a",
                          getLLVMStyleWithColumns(15));
   verifyFormat("#define A     \\\n"