clang-format: Add preprocessor directive indentation

[euhlmann]

Summary

This is an implementation for bug 17362 which adds support for indenting preprocessor statements inside if/ifdef/endif. This takes previous work from fmauch (https://github.com/fmauch/clang/tree/preprocessor_indent) and makes it into a full feature.
The context of this patch is that I'm a VMware intern, and I implemented this because VMware needs the feature. As such, some decisions were made based on what VMware wants, and I would appreciate suggestions on expanding this if necessary to use-cases other people may want.

This adds a new enum config option, IndentPPDirectives. Values are:

• PPDIS_None (in config: None):

#if FOO
#if BAR
#include <foo>
#endif
#endif

• PPDIS_AfterHash (in config: AfterHash):

#if FOO
#  if BAR
#    include <foo>
#  endif
#endif

This is meant to work whether spaces or tabs are used for indentation. Preprocessor indentation is independent of indentation for non-preprocessor lines.

Preprocessor indentation also attempts to ignore include guards with the checks:

1. Include guards cover the entire file
2. Include guards don't have #else
3. Include guards begin with

#ifndef <var>
#define <var>

This patch allows UnwrappedLineParser::PPBranchLevel to be decremented to -1 (the initial value is -1) so the variable can be used for indent tracking.

Defects:

• This patch does not handle the case where there's code between the #ifndef and #define but all other conditions hold. This is because when the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the previous code line yet, so we can't detect it. This is out of the scope of this patch.

• This patch does not handle cases where legitimate lines may be outside an include guard. Examples are #pragma once and #pragma GCC diagnostic, or anything else that does not change the meaning of the file if it's included multiple times.

• This does not detect when there is a single non-preprocessor line in front of an include-guard-like structure where other conditions hold because ScopedLineState hides the line.

• Preprocessor indentation throws off TokenAnnotator::setCommentLineLevels so the indentation of comments immediately before indented preprocessor lines is toggled on each run. Fixing this issue appears to be a major change and too much complexity for this patch.

[djasper]

docs/ClangFormatStyleOptions.rst

1182 ↗

(On Diff #108484)

I think we can foresee that a bool is not going to be enough here. Make this an enum, which for no can contain the values "no" and "afterhash" and in the long run can get a "beforehash" value.

lib/Format/ContinuationIndenter.cpp

379 ↗

(On Diff #108484)

Start comments upper case.

383 ↗

(On Diff #108484)

Same here and use full sentences.

Also, this does not seem to be what the example in the style option suggests and I'd rather not do it (subtracting 1).

lib/Format/UnwrappedLineFormatter.cpp

1021 ↗

(On Diff #108484)

... or not at all. :)

lib/Format/UnwrappedLineParser.cpp

667 ↗

(On Diff #108484)

LLVM style does not use braces for single statement ifs.

699 ↗

(On Diff #108484)

In LLVM's codebase, we mostly just leave out the "!= nullptr" and rely on the implicit conversion to bool.

701 ↗

(On Diff #108484)

I think you'll need substantially more tests here:

• An include guard must not have a #else
• An include guard must span the entire file

lib/Format/UnwrappedLineParser.h

238 ↗

(On Diff #108484)

I think this should almost always be PPBranchLevel. Probably the different case is the #else itself, but I think we can handle that easily.

[euhlmann]

lib/Format/ContinuationIndenter.cpp

383 ↗

(On Diff #108484)

I apologize, the style option documentation was a typo. The patch summary has the intended behavior, and I mentioned there that I count the hash as a column. Part of the reasoning for this is because it would look the same visually with spaces or tabs.

[euhlmann]

lib/Format/UnwrappedLineParser.cpp

701 ↗

(On Diff #108484)

It sounds like these would require two passes, since we wouldn't know until the end whether an include guard spans the whole file or has a #else. Do we really want two passes? Or do you have any advice on how these tests can be implemented?

[djasper]

lib/Format/ContinuationIndenter.cpp

383 ↗

(On Diff #108484)

Do you know of a coding style that writes something about this? I think the similarity of spaces vs. tabs is not a strong reason because a source file will either use one or the other. To me:

#if a == 1
# define X
# if b == 2
#   define Y
...

Would look weird. I'd prefer if we kept this simpler and more consistent.

lib/Format/UnwrappedLineParser.cpp

701 ↗

(On Diff #108484)

You could store an additional bool that gets set to true if the possible include guard is matched up with an appropriate #endif in the last line of a file. If that bool is true, you can remove 1 from the level of all preprocessor lines.

[euhlmann]

lib/Format/UnwrappedLineParser.h

238 ↗

(On Diff #108484)

It doesn't look like PPBranchLevel can be used to differentiate between no indent and one level of indent, since it starts at -1 and then stays >=0. For example:

#define A     // -1
#if B         // -1
#  define C   // 0
#  if D       // 0
#    define E // 1
#  endif      // 0
#endif        // 0
#define F     // 0

[djasper]

Manuel: Can you take a look at the last comment here? Why does PPBranchLevel start at -1?

[klimek]

In D35955#834914, @djasper wrote:

Manuel: Can you take a look at the last comment here? Why does PPBranchLevel start at -1?

It's a perhaps too-clever implementation to make sure that we can have a per-branch data structure (indexed from 0) , and then making sure that we never index out of bounds by never going down to -1 again.
I think if we need this info, we can just make it count down to -1 again (or, but that's isomorphic, let it run from 0 and make sure we never index into the data structures at 0 :)

[euhlmann]

In D35955#835439, @klimek wrote:

I think if we need this info, we can just make it count down to -1 again (or, but that's isomorphic, let it run from 0 and make sure we never index into the data structures at 0 :)

Should I do one of these things? Let me know how you'd like me to implement this.

[klimek]

In D35955#835994, @euhlmann wrote:

In D35955#835439, @klimek wrote:

I think if we need this info, we can just make it count down to -1 again (or, but that's isomorphic, let it run from 0 and make sure we never index into the data structures at 0 :)

Should I do one of these things? Let me know how you'd like me to implement this.

So my suggestion would be to let it count down to -1 again and put a check around indexing into the data structures.

[euhlmann]

This addresses Daniel's previous concerns. The option is now an enum, the heuristic for detecting include guards is expanded and has corresponding unit tests, and style issues are resolved.
This does not yet use PPBranchLevel to track indent; I'm updating this review with my current work before addressing that point.

There are minor defects in the implementation, which I've put in the change description and added EXPECT_NE tests for. I don't know how to fix those, and I'm not sure whether they should be considered important. Let me know if you think they need to be fixed.

[euhlmann]

The patch now uses PPBranchLevel to track indent level. It allows PPBranchLevel to go back down to -1. The existing PPBranchLevel >= 0 checks appear to prevent indexing into structures when PPBranchLevel is -1.

[klimek]

LG from my side.

[mzeren-vmw]

Daniel's right. We need need substantially more tests!

docs/ClangFormatStyleOptions.rst

1199 ↗

(On Diff #110429)

delete outdated bit of description: ", counting the hash as a column"

include/clang/Format/Format.h

1038 ↗

(On Diff #110429)

ditto

lib/Format/UnwrappedLineParser.cpp

707 ↗

(On Diff #110602)

... has an #else ...

728 ↗

(On Diff #110602)

You can use a range based for loop here. Also locals begin with upper case:

for (auto& Line : Lines)  {

742–744 ↗

(On Diff #110602)

Lines.size() == 1 is problematic. We must at least support comments before the include guard. I'll comment on the tests and point out a couple of important cases that currently fail.

unittests/Format/FormatTest.cpp

2281 ↗

(On Diff #110602)

Experimenting with the patch locally I found that comment indentation is broken in some cases. Please add tests that cover comments. For example:

comment indented at code level as expected:

void f() {
#if 0
  // Comment
  code();
#  define FOO 0
#endif
}

comment not indented at code level when there's a guard:

#ifndef _SOMEFILE_H
#define _SOMEFILE_H
void f() {
#if 0
// Comment
  code();
#  define FOO 0
#endif
}
#endif

The #define FOO 0 is required to trigger this behavior.

2322–2331 ↗

(On Diff #110602)

A DRY-er way to say this is:

auto WithGuard = "#ifndef _SOMEFILE_H\n"
                 "#define _SOMEFILE_H\n"
                 "code();\n"
                 "#endif";
ASSERT_EQ(WithGuards, format(WithGuards, Style));

2334 ↗

(On Diff #110602)

It would be nice to have a comment that explains why you cannot use verifyFormat.

2394 ↗

(On Diff #110602)

The two "Defect:" cases would be more precise and more self-documenting if they used EXPECT_EQ where the "expected" text showed the incorrect results.

2405–2408 ↗

(On Diff #110602)

We need to handle comments (like copyrights) before the include guard. There is an analogous problem with trailing blank lines, or trailing comments.

I think we need a small state machine: Init, Start, Open, Closed, NotGuard (terminal). FoundIncludeGuardStart should change from a bool to an enum to track this state. PPMaybeIncludeGuard can then drop it's "mabye". Whether or not it is null depends directly on the state. It does not determine state itself. While parsing, each line updates the state. If we get to eof in the Closed state then we have detected an include guard. ... Or similar logic....

Note that support for comments before the guard opens the door to support for #pragma once. It is tempting to add that, but that is a slippery slope. I would recommend leaving that as a defect that can be addressed later.

[mzeren-vmw]

unittests/Format/FormatTest.cpp

2281 ↗

(On Diff #110602)

Erik spent some time investigating issues with comment indentation. A couple of insights so far:

a) We need tests for wrapped macros (with trailing \ continuations). These need to include embedded comments.

b) It would be most natural to align comments with the following non-comment line. So a comment may be indented at "preprocessor level" or "code level". If indented at preprocessor level we have to handle the extra space introduced by the leading hash. This may require an extra bit per line to indicate whether the "aligned-to" line is preprocessor or code.

[euhlmann]

unittests/Format/FormatTest.cpp

2405–2408 ↗

(On Diff #110602)

@djasper @klimek
Do you have any comments on this? I've begun to implement an enum/state machine but I'd like to know if you'd like me to implement a different way.

[djasper]

unittests/Format/FormatTest.cpp

2281 ↗

(On Diff #110602)

"#if 0" specifically might not be a good candidate for a test case as that should be treated like a comment. We should not try to format anything inside a "#if 0" and I'd be surprised if clang-format touched that at all. "#if A" or something is more appropriate.

2322–2331 ↗

(On Diff #110602)

I think _A... is actually not an identifier that is ok to use. Remove the leading "_".

2334 ↗

(On Diff #110602)

Or even have a separate function that messes this up in some other way, not altering line breaks so that we can keep these tests short.

2405–2408 ↗

(On Diff #110602)

Most of the work here is done in the UnwrappedLineParser, which should skip over comments anyway. So yes, we should allow for these comments, but I don't think the logic needs to get much more complex because of that. I strongly doubt that a state machine is the right mechanism here.

And generally (without specific action to derive from that), I think we should try to err on the side of incorrectly finding header guards instead of incorrectly not recognizing a header guard. I think a not-indented #define somewhere is better than an incorrectly indented header guard.

[euhlmann]

Allows comments before the include guard opens. However, if there's a single non-comment line before an include-guard-like structure that meets all the other conditions then it will be treated as an include guard because Lines does not yet contain the line before the #ifndef. This looks difficult to fix and I believe it's a rare enough situation to ignore.

This does not yet address avoiding repetition in the tests or comments before indented preprocessor lines being indented.

[euhlmann]

Eliminated redundancy in tests by fixing test::messUp which was incorrectly merging lines.

Fixing the issue Mark brought up regarding comments before indented preprocessor lines appears to be a major change and I believe it's too much complexity for this patch.

[djasper]

Krasimir: Can you actually give this a round of review? I will also try to do so tomorrow.

[krasimir]

lib/Format/ContinuationIndenter.cpp

383 ↗

(On Diff #111606)

You can replace Current.Previous with Previous. Also, I'd swap the checks a bit, like:

if (Previous.is(tok::hash) && State.FirstIndent > 0 &&
    Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash &&
    (State.Line->Type == LT_PreprocessorDirective ||
     State.Line->Type == LT_ImportStatement)) {

That way, the common case Previous.is(tok::hash) == false is handled quickly.

387 ↗

(On Diff #111606)

I don't understand this comment. Could you please give an example?

lib/Format/UnwrappedLineParser.cpp

701 ↗

(On Diff #111606)

This can easily lead to a pretty bad runtime: consider a file starting with a few hundred lines of comments and having a few hundred #ifdef-s.

I'd say that after we do this MaybeIncludeGuard thing once, we don't repeat it again.

Also, lines could start with a block comment and continue with code:

/* small */ int ten = 10;

732 ↗

(On Diff #111606)

Why do we need to subtract one from every preprocessor indent?

737 ↗

(On Diff #111606)

Wouldn't this also indent lines continuing macro definitions, as in:

#define A(x) int f(int x) { \
  return x; \
}

760 ↗

(On Diff #111606)

Why do we reset PPMaybeIncludeGuard here?

770 ↗

(On Diff #111606)

Why do we ++Line->Level here?

787 ↗

(On Diff #111606)

Why do we reset PPMaybeIncludeGuard here?

lib/Format/UnwrappedLineParser.h

249 ↗

(On Diff #111606)

Please add a comment for PPMaybeIncludeGuard: is it expected to point to the hash token of the #ifdef, or?

[euhlmann]

lib/Format/ContinuationIndenter.cpp

387 ↗

(On Diff #111606)

When tabs are enabled, extra spaces would be added because of the column offset caused by the # token.

#if FOO
#<tab>if BAR
#<tab><space>define BAZ
#<tab>endif
#endif

Let me know if there's a better way to fix that.

lib/Format/UnwrappedLineParser.cpp

732 ↗

(On Diff #111606)

This only knows for sure when something is an include guard at the end of the file. By that time, everything inside the include guard has already been indented one extra level since we didn't know about the guard yet, so it would look like

#ifndef FILE_H
#  define FILE_H
// ...
#  other directives
// ...
#endif

Subtracting one is the action taken when we decide that there's a valid include guard, so it looks like

#ifndef FILE_H
#define FILE_H
// ...
#other directives
// ...
#endif

760 ↗

(On Diff #111606)

The idea is if there's any preprocessor lines between the #ifndef and the #define, it doesn't treat it as an include guard. This is the same reason for the other PPMaybeIncludeGuard reset.

770 ↗

(On Diff #111606)

This sets correct indent for multi-line macros, so they are indented one level more than the indent of the #define instead of always at level 1.

[djasper]

unittests/Format/FormatTest.cpp

2297 ↗

(On Diff #111606)

Please just set Style.ColumnLimit to 60 or so in order not to have to wrap the test cases. Makes then much easier to understand.

2314 ↗

(On Diff #111606)

It's hard to tell with the line wrap, but this does look wrong, the escaped newline does not seem to be in column 80.

2427 ↗

(On Diff #111606)

Write "FIXME" instead of "Defect"

unittests/Format/FormatTestUtils.h

34 ↗

(On Diff #111606)

Could we just do:

} else if (MessedUp[i] == '#' && (JustReplacedNewline || i == 0 || MessedUp[i - 1] == '\n') {

?

[euhlmann]

• Swapped checks in ContinuationIndenter
• Renamed PPMaybeIncludeGuard to IfNdefCondition and added comment
• Added another bool member that tracks if include guard detection was rejected once. If the bool gets set, the loop checking previous lines doesn't run again.
• Set lower ColumnLimit in tests
• Fixed incorrect column for \ in multi-line macros
• Resolved comment in FormatTestUtils

[euhlmann]

lib/Format/UnwrappedLineParser.cpp

737 ↗

(On Diff #111606)

Multi-line macros look this under this patch

#if A
#  define MACRO                        \
    void a(int x) {                    \
      b();                             \
      c();                             \
      d();                             \
      e();                             \
      f();                             \
    }
#endif

With previous clang-format, no preprocessor indentation:

#if A
#define MACRO                          \
  void a(int x) {                      \
    b();                               \
    c();                               \
    d();                               \
    e();                               \
    f();                               \
  }
#endif

[mzeren-vmw]

lib/Format/ContinuationIndenter.cpp

383 ↗

(On Diff #108484)

@djasper I just noticed that the Google Style Guide has:

#if DISASTER_PENDING      // Correct -- Starts at beginning of line
    DropEverything();
# if NOTIFY               // OK but not required -- Spaces after #
    NotifyClient();
# endif
#endif

Note the single space in # if NOTIFY. Can we correct the Guide to match what we have here?

[djasper]

From my side this looks good for now (we can always improve more later). Krasimir, what do you think?

[krasimir]

Thank you! I understand this patch better now. Looks good from my side!

[euhlmann]

I'm glad this is finally in a state to land. Thanks for the helpful reviews!

[krasimir]

@euhlmann: are you planning to commit this?

[krasimir]

I'll land this for you, as discussed offline. The best thing is to apply for commit rights after you have a few patches landed.

cfe/trunk/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
      switch (fool) {                vs.     switch (fool) {
      case 1:                                  case 1:
        bar();                                   bar();
        break;                                   break;
      default:                                 default:
        plop();                                  plop();
      }                                      }
 
+**IndentPPDirectives** (``PPDirectiveIndentStyle``)
+  Indent preprocessor directives on conditionals.
+
+  Possible values:
+
+  * ``PPDIS_None`` (in configuration: ``None``)
+    Does not indent any directives.
+
+    .. code-block:: c++
+
+      #if FOO
+      #if BAR
+      #include <foo>
+      #endif
+      #endif
+
+  * ``PPDIS_AfterHash`` (in configuration: ``AfterHash``)
+    Indents directives after the hash.
+
+    .. code-block:: c++
+
+      #if FOO
+      #  if BAR
+      #    include <foo>
+      #  endif
+      #endif
+
 **IndentWidth** (``unsigned``)
   The number of columns to use for indentation.
 
   .. code-block:: c++
 
      IndentWidth: 3
 
      void f() {

cfe/trunk/include/clang/Format/Format.h

struct FormatStyle {
   ///      bar();                                   bar();
   ///      break;                                   break;
   ///    default:                                 default:
   ///      plop();                                  plop();
   ///    }                                      }
   /// \endcode
   bool IndentCaseLabels;
 
+  /// \brief Options for indenting preprocessor directives.
+  enum PPDirectiveIndentStyle {
+    /// Does not indent any directives.
+    /// \code
+    ///    #if FOO
+    ///    #if BAR
+    ///    #include <foo>
+    ///    #endif
+    ///    #endif
+    /// \endcode
+    PPDIS_None,
+    /// Indents directives after the hash.
+    /// \code
+    ///    #if FOO
+    ///    #  if BAR
+    ///    #    include <foo>
+    ///    #  endif
+    ///    #endif
+    /// \endcode
+    PPDIS_AfterHash
+  };
+
+  /// \brief The preprocessor directive indenting style to use.
+  PPDirectiveIndentStyle IndentPPDirectives;
+
   /// \brief The number of columns to use for indentation.
   /// \code
   ///    IndentWidth: 3
   ///
   ///    void f() {
   ///       someFunction();
   ///       if (true, false) {
   ///          f();
return AccessModifierOffset == R.AccessModifierOffset &&
            DerivePointerAlignment == R.DerivePointerAlignment &&
            DisableFormat == R.DisableFormat &&
            ExperimentalAutoDetectBinPacking ==
                R.ExperimentalAutoDetectBinPacking &&
            FixNamespaceComments == R.FixNamespaceComments &&
            ForEachMacros == R.ForEachMacros &&
            IncludeCategories == R.IncludeCategories &&
            IndentCaseLabels == R.IndentCaseLabels &&
+           IndentPPDirectives == R.IndentPPDirectives &&
            IndentWidth == R.IndentWidth && Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
            JavaScriptQuotes == R.JavaScriptQuotes &&
            JavaScriptWrapImports == R.JavaScriptWrapImports &&
            KeepEmptyLinesAtTheStartOfBlocks ==
                R.KeepEmptyLinesAtTheStartOfBlocks &&
            MacroBlockBegin == R.MacroBlockBegin &&
            MacroBlockEnd == R.MacroBlockEnd &&

cfe/trunk/lib/Format/ContinuationIndenter.cpp

: Style(Style), Keywords(Keywords), SourceMgr(SourceMgr),
       CommentPragmasRegex(Style.CommentPragmas) {}
 
 LineState ContinuationIndenter::getInitialState(unsigned FirstIndent,
                                                 const AnnotatedLine *Line,
                                                 bool DryRun) {
   LineState State;
   State.FirstIndent = FirstIndent;
   State.Column = FirstIndent;
+  // With preprocessor directive indentation, the line starts on column 0
+  // since it's indented after the hash, but FirstIndent is set to the
+  // preprocessor indent.
+  if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash &&
+      (Line->Type == LT_PreprocessorDirective ||
+       Line->Type == LT_ImportStatement))
+    State.Column = 0;
   State.Line = Line;
   State.NextToken = Line->First;
   State.Stack.push_back(ParenState(FirstIndent, FirstIndent,
                                    /*AvoidBinPacking=*/false,
                                    /*NoLineBreak=*/false));
   State.LineContainsContinuedForLoopSection = false;
   State.StartOfStringLiteral = 0;
   State.StartOfLineLevel = 0;
while (Tok && State.Stack.back().VariablePos >= Tok->ColumnWidth) {
       Tok = Tok->Previous;
     }
     if (Previous.PartOfMultiVariableDeclStmt)
       State.Stack.back().LastSpace = State.Stack.back().VariablePos;
   }
 
   unsigned Spaces = Current.SpacesRequiredBefore + ExtraSpaces;
 
+  // Indent preprocessor directives after the hash if required.
+  int PPColumnCorrection = 0;
+  if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash &&
+      Previous.is(tok::hash) && State.FirstIndent > 0 &&
+      (State.Line->Type == LT_PreprocessorDirective ||
+       State.Line->Type == LT_ImportStatement)) {
+    Spaces += State.FirstIndent;
+
+    // For preprocessor indent with tabs, State.Column will be 1 because of the
+    // hash. This causes second-level indents onward to have an extra space
+    // after the tabs. We avoid this misalignment by subtracting 1 from the
+    // column value passed to replaceWhitespace().
+    if (Style.UseTab != FormatStyle::UT_Never)
+      PPColumnCorrection = -1;
+  }
+
   if (!DryRun)
     Whitespaces.replaceWhitespace(Current, /*Newlines=*/0, Spaces,
-                                  State.Column + Spaces);
+                                  State.Column + Spaces + PPColumnCorrection);
 
   // If "BreakBeforeInheritanceComma" mode, don't break within the inheritance
   // declaration unless there is multiple inheritance.
   if (Style.BreakBeforeInheritanceComma && Current.is(TT_InheritanceColon))
     State.Stack.back().NoLineBreak = true;
 
   if (Current.is(TT_SelectorName) &&
       !State.Stack.back().ObjCSelectorNameFound) {

cfe/trunk/lib/Format/Format.cpp

template <> struct ScalarEnumerationTraits<FormatStyle::BreakConstructorInitializersStyle> {
   static void enumeration(IO &IO, FormatStyle::BreakConstructorInitializersStyle &Value) {
     IO.enumCase(Value, "BeforeColon", FormatStyle::BCIS_BeforeColon);
     IO.enumCase(Value, "BeforeComma", FormatStyle::BCIS_BeforeComma);
     IO.enumCase(Value, "AfterColon", FormatStyle::BCIS_AfterColon);
   }
 };
 
 template <>
+struct ScalarEnumerationTraits<FormatStyle::PPDirectiveIndentStyle> {
+  static void enumeration(IO &IO, FormatStyle::PPDirectiveIndentStyle &Value) {
+    IO.enumCase(Value, "None", FormatStyle::PPDIS_None);
+    IO.enumCase(Value, "AfterHash", FormatStyle::PPDIS_AfterHash);
+  }
+};
+
+template <>
 struct ScalarEnumerationTraits<FormatStyle::ReturnTypeBreakingStyle> {
   static void enumeration(IO &IO, FormatStyle::ReturnTypeBreakingStyle &Value) {
     IO.enumCase(Value, "None", FormatStyle::RTBS_None);
     IO.enumCase(Value, "All", FormatStyle::RTBS_All);
     IO.enumCase(Value, "TopLevel", FormatStyle::RTBS_TopLevel);
     IO.enumCase(Value, "TopLevelDefinitions",
                 FormatStyle::RTBS_TopLevelDefinitions);
     IO.enumCase(Value, "AllDefinitions", FormatStyle::RTBS_AllDefinitions);
static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("DisableFormat", Style.DisableFormat);
     IO.mapOptional("ExperimentalAutoDetectBinPacking",
                    Style.ExperimentalAutoDetectBinPacking);
     IO.mapOptional("FixNamespaceComments", Style.FixNamespaceComments);
     IO.mapOptional("ForEachMacros", Style.ForEachMacros);
     IO.mapOptional("IncludeCategories", Style.IncludeCategories);
     IO.mapOptional("IncludeIsMainRegex", Style.IncludeIsMainRegex);
     IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
+    IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
     IO.mapOptional("IndentWidth", Style.IndentWidth);
     IO.mapOptional("IndentWrappedFunctionNames",
                    Style.IndentWrappedFunctionNames);
     IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
     IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
     IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
                    Style.KeepEmptyLinesAtTheStartOfBlocks);
     IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);
FormatStyle getLLVMStyle() {
   LLVMStyle.ForEachMacros.push_back("foreach");
   LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
   LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
   LLVMStyle.IncludeCategories = {{"^\"(llvm|llvm-c|clang|clang-c)/", 2},
                                  {"^(<|\"(gtest|gmock|isl|json)/)", 3},
                                  {".*", 1}};
   LLVMStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IndentCaseLabels = false;
+  LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;

cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp

if (RootToken.is(tok::r_brace) &&
       (!RootToken.Next ||
        (RootToken.Next->is(tok::semi) && !RootToken.Next->Next)))
     Newlines = std::min(Newlines, 1u);
   if (Newlines == 0 && !RootToken.IsFirst)
     Newlines = 1;
   if (RootToken.IsFirst && !RootToken.HasUnescapedNewline)
     Newlines = 0;
 
+  // Preprocessor directives get indented after the hash, if indented.
+  if (Line.Type == LT_PreprocessorDirective || Line.Type == LT_ImportStatement)
+    Indent = 0;
+
   // Remove empty lines after "{".
   if (!Style.KeepEmptyLinesAtTheStartOfBlocks && PreviousLine &&
       PreviousLine->Last->is(tok::l_brace) &&
       PreviousLine->First->isNot(tok::kw_namespace) &&
       !startsExternCBlock(*PreviousLine))
     Newlines = 1;
 
   // Insert extra new line before access specifiers.

cfe/trunk/lib/Format/UnwrappedLineParser.h

private:
   SmallVector<int, 8> PPLevelBranchCount;
 
   // Contains the number of branches per nesting level we are currently
   // in while parsing a preprocessor branch sequence.
   // This is used to update PPLevelBranchCount at the end of a branch
   // sequence.
   std::stack<int> PPChainBranchIndex;
 
+  // Contains the #ifndef condition for a potential include guard.
+  FormatToken *IfNdefCondition;
+  bool FoundIncludeGuardStart;
+  bool IncludeGuardRejected;
+
   friend class ScopedLineState;
   friend class CompoundStatementIndenter;
 };
 
 struct UnwrappedLineNode {
   UnwrappedLineNode() : Tok(nullptr) {}
   UnwrappedLineNode(FormatToken *Tok) : Tok(Tok) {}
 

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

 
 UnwrappedLineParser::UnwrappedLineParser(const FormatStyle &Style,
                                          const AdditionalKeywords &Keywords,
                                          ArrayRef<FormatToken *> Tokens,
                                          UnwrappedLineConsumer &Callback)
     : Line(new UnwrappedLine), MustBreakBeforeNextToken(false),
       CurrentLines(&Lines), Style(Style), Keywords(Keywords),
       CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
-      Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1) {}
+      Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
+      IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
+      IncludeGuardRejected(false) {}
 
 void UnwrappedLineParser::reset() {
   PPBranchLevel = -1;
+  IfNdefCondition = nullptr;
+  FoundIncludeGuardStart = false;
+  IncludeGuardRejected = false;
   Line.reset(new UnwrappedLine);
   CommentsBeforeNextToken.clear();
   FormatTok = nullptr;
   MustBreakBeforeNextToken = false;
   PreprocessorDirectives.clear();
   CurrentLines = &Lines;
   DeclarationScopeStack.clear();
   PPStack.clear();

 void UnwrappedLineParser::conditionalCompilationEnd() {
   assert(PPBranchLevel < (int)PPLevelBranchIndex.size());
   if (PPBranchLevel >= 0 && !PPChainBranchIndex.empty()) {
     if (PPChainBranchIndex.top() + 1 > PPLevelBranchCount[PPBranchLevel]) {
       PPLevelBranchCount[PPBranchLevel] = PPChainBranchIndex.top() + 1;
     }
   }
   // Guard against #endif's without #if.
-  if (PPBranchLevel > 0)
+  if (PPBranchLevel > -1)
     --PPBranchLevel;
   if (!PPChainBranchIndex.empty())
     PPChainBranchIndex.pop();
   if (!PPStack.empty())
     PPStack.pop_back();
 }
 
 void UnwrappedLineParser::parsePPIf(bool IfDef) {
   bool IfNDef = FormatTok->is(tok::pp_ifndef);
   nextToken();
   bool Unreachable = false;
   if (!IfDef && (FormatTok->is(tok::kw_false) || FormatTok->TokenText == "0"))
     Unreachable = true;
   if (IfDef && !IfNDef && FormatTok->TokenText == "SWIG")
     Unreachable = true;
   conditionalCompilationStart(Unreachable);
+  FormatToken *IfCondition = FormatTok;
+  // If there's a #ifndef on the first line, and the only lines before it are
+  // comments, it could be an include guard.
+  bool MaybeIncludeGuard = IfNDef;
+  if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard) {
+    for (auto &Line : Lines) {
+      if (!Line.Tokens.front().Tok->is(tok::comment)) {
+        MaybeIncludeGuard = false;
+        IncludeGuardRejected = true;
+        break;
+      }
+    }
+  }
+  --PPBranchLevel;
   parsePPUnknown();
+  ++PPBranchLevel;
+  if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard)
+    IfNdefCondition = IfCondition;
 }
 
 void UnwrappedLineParser::parsePPElse() {
+  // If a potential include guard has an #else, it's not an include guard.
+  if (FoundIncludeGuardStart && PPBranchLevel == 0)
+    FoundIncludeGuardStart = false;
   conditionalCompilationAlternative();
+  if (PPBranchLevel > -1)
+    --PPBranchLevel;
   parsePPUnknown();
+  ++PPBranchLevel;
 }
 
 void UnwrappedLineParser::parsePPElIf() { parsePPElse(); }
 
 void UnwrappedLineParser::parsePPEndIf() {
   conditionalCompilationEnd();
   parsePPUnknown();
+  // If the #endif of a potential include guard is the last thing in the file,
+  // then we count it as a real include guard and subtract one from every
+  // preprocessor indent.
+  unsigned TokenPosition = Tokens->getPosition();
+  FormatToken *PeekNext = AllTokens[TokenPosition];
+  if (FoundIncludeGuardStart && PPBranchLevel == -1 && PeekNext->is(tok::eof)) {
+    for (auto &Line : Lines) {
+      if (Line.InPPDirective && Line.Level > 0)
+        --Line.Level;
+    }
+  }
 }
 
 void UnwrappedLineParser::parsePPDefine() {
   nextToken();
 
   if (FormatTok->Tok.getKind() != tok::identifier) {
     parsePPUnknown();
     return;
   }
+  if (IfNdefCondition && IfNdefCondition->TokenText == FormatTok->TokenText) {
+    FoundIncludeGuardStart = true;
+    for (auto &Line : Lines) {
+      if (!Line.Tokens.front().Tok->isOneOf(tok::comment, tok::hash)) {
+        FoundIncludeGuardStart = false;
+        break;
+      }
+    }
+  }
+  IfNdefCondition = nullptr;
   nextToken();
   if (FormatTok->Tok.getKind() == tok::l_paren &&
       FormatTok->WhitespaceRange.getBegin() ==
           FormatTok->WhitespaceRange.getEnd()) {
     parseParens();
   }
+  if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash)
+    Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
-  Line->Level = 1;
+  ++Line->Level;
 
   // 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();
 }
 
 void UnwrappedLineParser::parsePPUnknown() {
   do {
     nextToken();
   } while (!eof());
+  if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash)
+    Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
+  IfNdefCondition = nullptr;
 }
 
 // Here we blacklist certain tokens that are not usually the first token in an
 // unwrapped line. This is used in attempt to distinguish macro calls without
 // trailing semicolons from other constructs split to several lines.
 static bool tokenCanStartNewLine(const clang::Token &Tok) {
   // Semicolon can be a null-statement, l_square can be a start of a macro or
   // a C++11 attribute, but this doesn't seem to be common.

cfe/trunk/unittests/Format/FormatTest.cpp

 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) {
   verifyFormat("#define A \\\n"
                "  f({     \\\n"
                "    g();  \\\n"
                "  });",
                getLLVMStyleWithColumns(11));
 }
 
-TEST_F(FormatTest, IndentPreprocessorDirectivesAtZero) {
-  EXPECT_EQ("{\n  {\n#define A\n  }\n}", format("{{\n#define A\n}}"));
+TEST_F(FormatTest, IndentPreprocessorDirectives) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  Style.ColumnLimit = 40;
+  verifyFormat("#ifdef _WIN32\n"
+               "#define A 0\n"
+               "#ifdef VAR2\n"
+               "#define B 1\n"
+               "#include <someheader.h>\n"
+               "#define MACRO                          \\\n"
+               "  some_very_long_func_aaaaaaaaaa();\n"
+               "#endif\n"
+               "#else\n"
+               "#define A 1\n"
+               "#endif",
+               Style);
+
+  Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef _WIN32\n"
+               "#  define A 0\n"
+               "#  ifdef VAR2\n"
+               "#    define B 1\n"
+               "#    include <someheader.h>\n"
+               "#    define MACRO                      \\\n"
+               "      some_very_long_func_aaaaaaaaaa();\n"
+               "#  endif\n"
+               "#else\n"
+               "#  define A 1\n"
+               "#endif",
+               Style);
+  verifyFormat("#if A\n"
+               "#  define MACRO                        \\\n"
+               "    void a(int x) {                    \\\n"
+               "      b();                             \\\n"
+               "      c();                             \\\n"
+               "      d();                             \\\n"
+               "      e();                             \\\n"
+               "      f();                             \\\n"
+               "    }\n"
+               "#endif",
+               Style);
+  // Comments before include guard.
+  verifyFormat("// file comment\n"
+               "// file comment\n"
+               "#ifndef HEADER_H\n"
+               "#define HEADER_H\n"
+               "code();\n"
+               "#endif",
+               Style);
+  // Test with include guards.
+  // EXPECT_EQ is used because verifyFormat() calls messUp() which incorrectly
+  // merges lines.
+  verifyFormat("#ifndef HEADER_H\n"
+               "#define HEADER_H\n"
+               "code();\n"
+               "#endif",
+               Style);
+  // Include guards must have a #define with the same variable immediately
+  // after #ifndef.
+  verifyFormat("#ifndef NOT_GUARD\n"
+               "#  define FOO\n"
+               "code();\n"
+               "#endif",
+               Style);
+
+  // Include guards must cover the entire file.
+  verifyFormat("code();\n"
+               "code();\n"
+               "#ifndef NOT_GUARD\n"
+               "#  define NOT_GUARD\n"
+               "code();\n"
+               "#endif",
+               Style);
+  verifyFormat("#ifndef NOT_GUARD\n"
+               "#  define NOT_GUARD\n"
+               "code();\n"
+               "#endif\n"
+               "code();",
+               Style);
+  // Test with trailing blank lines.
+  verifyFormat("#ifndef HEADER_H\n"
+               "#define HEADER_H\n"
+               "code();\n"
+               "#endif\n",
+               Style);
+  // Include guards don't have #else.
+  verifyFormat("#ifndef NOT_GUARD\n"
+               "#  define NOT_GUARD\n"
+               "code();\n"
+               "#else\n"
+               "#endif",
+               Style);
+  verifyFormat("#ifndef NOT_GUARD\n"
+               "#  define NOT_GUARD\n"
+               "code();\n"
+               "#elif FOO\n"
+               "#endif",
+               Style);
+  // FIXME: This doesn't handle the case where there's code between the
+  // #ifndef and #define but all other conditions hold. This is because when
+  // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
+  // previous code line yet, so we can't detect it.
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+            "code();\n"
+            "#define NOT_GUARD\n"
+            "code();\n"
+            "#endif",
+            format("#ifndef NOT_GUARD\n"
+                   "code();\n"
+                   "#  define NOT_GUARD\n"
+                   "code();\n"
+                   "#endif",
+                   Style));
+  // FIXME: This doesn't handle cases where legitimate preprocessor lines may
+  // be outside an include guard. Examples are #pragma once and
+  // #pragma GCC diagnostic, or anything else that does not change the meaning
+  // of the file if it's included multiple times.
+  EXPECT_EQ("#ifdef WIN32\n"
+            "#  pragma once\n"
+            "#endif\n"
+            "#ifndef HEADER_H\n"
+            "#  define HEADER_H\n"
+            "code();\n"
+            "#endif",
+            format("#ifdef WIN32\n"
+                   "#  pragma once\n"
+                   "#endif\n"
+                   "#ifndef HEADER_H\n"
+                   "#define HEADER_H\n"
+                   "code();\n"
+                   "#endif",
+                   Style));
+  // FIXME: This does not detect when there is a single non-preprocessor line
+  // in front of an include-guard-like structure where other conditions hold
+  // because ScopedLineState hides the line.
+  EXPECT_EQ("code();\n"
+            "#ifndef HEADER_H\n"
+            "#define HEADER_H\n"
+            "code();\n"
+            "#endif",
+            format("code();\n"
+                   "#ifndef HEADER_H\n"
+                   "#  define HEADER_H\n"
+                   "code();\n"
+                   "#endif",
+                   Style));
+  // FIXME: The comment indent corrector in TokenAnnotator gets thrown off by
+  // preprocessor indentation.
+  EXPECT_EQ("#if 1\n"
+            "  // comment\n"
+            "#  define A 0\n"
+            "// comment\n"
+            "#  define B 0\n"
+            "#endif",
+            format("#if 1\n"
+                   "// comment\n"
+                   "#  define A 0\n"
+                   "   // comment\n"
+                   "#  define B 0\n"
+                   "#endif",
+                   Style));
+  // Test with tabs.
+  Style.UseTab = FormatStyle::UT_Always;
+  Style.IndentWidth = 8;
+  Style.TabWidth = 8;
+  verifyFormat("#ifdef _WIN32\n"
+               "#\tdefine A 0\n"
+               "#\tifdef VAR2\n"
+               "#\t\tdefine B 1\n"
+               "#\t\tinclude <someheader.h>\n"
+               "#\t\tdefine MACRO          \\\n"
+               "\t\t\tsome_very_long_func_aaaaaaaaaa();\n"
+               "#\tendif\n"
+               "#else\n"
+               "#\tdefine A 1\n"
+               "#endif",
+               Style);
 }
 
 TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) {
   verifyFormat("{\n  { a #c; }\n}");
 }
 
 TEST_F(FormatTest, FormatUnbalancedStructuralElements) {
   EXPECT_EQ("#define A \\\n  {       \\\n    {\nint i;",

cfe/trunk/unittests/Format/FormatTestUtils.h

inline std::string messUp(llvm::StringRef Code) {
   bool InComment = false;
   bool InPreprocessorDirective = false;
   bool JustReplacedNewline = false;
   for (unsigned i = 0, e = MessedUp.size() - 1; i != e; ++i) {
     if (MessedUp[i] == '/' && MessedUp[i + 1] == '/') {
       if (JustReplacedNewline)
         MessedUp[i - 1] = '\n';
       InComment = true;
-    } else if (MessedUp[i] == '#' && (JustReplacedNewline || i == 0)) {
+    } else if (MessedUp[i] == '#' &&
+               (JustReplacedNewline || i == 0 || MessedUp[i - 1] == '\n')) {
       if (i != 0)
         MessedUp[i - 1] = '\n';
       InPreprocessorDirective = true;
     } else if (MessedUp[i] == '\\' && MessedUp[i + 1] == '\n') {
       MessedUp[i] = ' ';
       MessedUp[i + 1] = ' ';
     } else if (MessedUp[i] == '\n') {
       if (InComment) {