[clang-format] BeforeHash added to IndentPPDirectives

[to-miz]

Summary

The option BeforeHash added to IndentPPDirectives.
Fixes Bug 36019. https://bugs.llvm.org/show_bug.cgi?id=36019

[to-miz]

PPBranchLevel can be negative, while Line->Level is unsigned. Added check to ensure that PPBranchLevel is positive before adding to Line->Level.

[krasimir]

As per https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options, could you please provide more info about the need for this option first?

[ufna]

@krasimir BeforeHash style is widely used for Unreal Engine 4 based projects development. I was looking for this option too.

[geleji]

I would find this useful too.

[to-miz]

I understand that adding new options adds costs to the development of clang-format, but there was a feature request and there is interest for this option.
As for the cost to the development, there already exists PPDIS_AfterHash. All I did was make the changes introduced in that patch a bit more general and removed the hard coded assumption that there are no indents before a hash at a couple of places.
I would argue the cost of development of this new option is roughly the same as it is with the existence of PPDIS_AfterHash itself. PPDIS_BeforeHash seems like a natural extension of the changes introduced by PPDIS_AfterHash.

[MyDeveloperDay]

Could you add some text into the docs/ReleaseNotes.rst to say you are adding a new option

take a look at https://clang.llvm.org/extra/ReleaseNotes.html#improvements-to-clang-tidy for an example

[MyDeveloperDay]

lib/Format/UnwrappedLineFormatter.cpp

15 ↗

(On Diff #165700)

Nice! its the irony of clang-format code is that its not clang formatted!!

[MyDeveloperDay]

While we wait for code review I've landed this currently unaccepted clang-format revision into https://github.com/mydeveloperday/clang-experimental/releases for those wishing to try/test or provide feedback

[to-miz]

Added an entry to ReleaseNotes.rst about the new option.
This diff doesn't contain unrelated formatting changes due to running clang-format on some source files that apparently weren't formatted before.

[MyDeveloperDay]

Thank you for helping to address one of the many clang-format issues from bugzilla, I'm not the code owner here but this LGTM.

[klimek]

Given this doesn't add an extra option (but only an extra enum) and is fairly straight forward, LG.

[MyDeveloperDay]

@to-miz do you have permission to commit?

[to-miz]

I don't have commit access. It would be nice if someone could commit it in my place.

[kadircet]

Could you do a rebase? I've tried to land it but there are too many merge conflicts currently.

[to-miz]

Is it because the diff was made with the svn repository? It is for the newest revision.
Should I make a new diff with the git repo instead?
Or is it not because of the diff, but something in phabricator instead?
Sorry this is my first patch.

[kadircet]

In D52150#1436278, @to-miz wrote:

Is it because the diff was made with the svn repository? It is for the newest revision.
Should I make a new diff with the git repo instead?
Or is it not because of the diff, but something in phabricator instead?
Sorry this is my first patch.

No worries. It is just because you've made your last update at 2nd of march(assuming you've rebased before that, otherwise it might be as old as last september) and its 20th today, there are other people making changes, which simply conflicts with yours.

[to-miz]

I checked out the git repository and applied the patch to head and reran all tests.
The diff produced by git is basically the same, should I upload the new diff?

[MyDeveloperDay]

In D52150#1436423, @to-miz wrote:

I checked out the git repository and applied the patch to head and reran all tests.
The diff produced by git is basically the same, should I upload the new diff?

I think you might need a new diff, I tried to merge this and it rejected some of the patch

[to-miz]

Made a new diff on the git repository, since the docs now recommend using git.
Rebased and created a new diff with git show HEAD -U999999.
The patch applies cleanly on windows after git reset --hard so I hope there shouldn't be any merge conflicts now.

cfe/trunk/docs/ClangFormatStyleOptions.rst

* ``PPDIS_AfterHash`` (in configuration: ``AfterHash``)
     .. code-block:: c++
 
        #if FOO
        #  if BAR
        #    include <foo>
        #  endif
        #endif
 
+  * ``PPDIS_BeforeHash`` (in configuration: ``BeforeHash``)
+    Indents directives before 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

cfe/trunk/docs/ReleaseNotes.rst

 AST Matchers
 ------------
 
 - ...
 
 clang-format
 ------------
 
-
-- ...
+- Added new option `PPDIS_BeforeHash` (in configuration: `BeforeHash`) to
+ `IndentPPDirectives` which indents preprocessor directives before the hash.
 
 libclang
 --------
 
 - When `CINDEXTEST_INCLUDE_ATTRIBUTED_TYPES` is not provided when making a
   CXType, the equivalent type of the AttributedType is returned instead of the
   modified type if the user does not want attribute sugar. The equivalent type
   represents the minimally-desugared type which the AttributedType is

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

enum PPDirectiveIndentStyle {
     /// Indents directives after the hash.
     /// \code
     ///    #if FOO
     ///    #  if BAR
     ///    #    include <foo>
     ///    #  endif
     ///    #endif
     /// \endcode
-    PPDIS_AfterHash
+    PPDIS_AfterHash,
+    /// Indents directives before the hash.
+    /// \code
+    ///    #if FOO
+    ///      #if BAR
+    ///        #include <foo>
+    ///      #endif
+    ///    #endif
+    /// \endcode
+    PPDIS_BeforeHash
   };
 
   /// The preprocessor directive indenting style to use.
   PPDirectiveIndentStyle IndentPPDirectives;
 
   /// The number of columns to use for indentation.
   /// \code
   ///    IndentWidth: 3

cfe/trunk/lib/Format/Format.cpp

struct ScalarEnumerationTraits<FormatStyle::BreakInheritanceListStyle> {
   }
 };
 
 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);
+    IO.enumCase(Value, "BeforeHash", FormatStyle::PPDIS_BeforeHash);
   }
 };
 
 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);

cfe/trunk/lib/Format/TokenAnnotator.cpp

for (SmallVectorImpl<AnnotatedLine *>::reverse_iterator I = Lines.rbegin(),
     }
 
     // If the comment is currently aligned with the line immediately following
     // it, that's probably intentional and we should keep it.
     if (NextNonCommentLine && CommentLine &&
         NextNonCommentLine->First->NewlinesBefore <= 1 &&
         NextNonCommentLine->First->OriginalColumn ==
             (*I)->First->OriginalColumn) {
-      // Align comments for preprocessor lines with the # in column 0.
-      // Otherwise, align with the next line.
-      (*I)->Level = (NextNonCommentLine->Type == LT_PreprocessorDirective ||
-                     NextNonCommentLine->Type == LT_ImportStatement)
-                        ? 0
-                        : NextNonCommentLine->Level;
+      // Align comments for preprocessor lines with the # in column 0 if
+      // preprocessor lines are not indented. Otherwise, align with the next
+      // line.
+      (*I)->Level =
+          (Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
+           (NextNonCommentLine->Type == LT_PreprocessorDirective ||
+            NextNonCommentLine->Type == LT_ImportStatement))
     } else {
       NextNonCommentLine = (*I)->First->isNot(tok::r_brace) ? (*I) : nullptr;
     }
 
     setCommentLineLevels((*I)->Children);
   }
 }
 

cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp

void UnwrappedLineFormatter::formatFirstToken(
   // Remove empty lines after access specifiers.
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
       (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
     Newlines = std::min(1u, Newlines);
 
   if (Newlines)
     Indent = NewlineIndent;
 
-  // Preprocessor directives get indented after the hash, if indented.
-  if (Line.Type == LT_PreprocessorDirective || Line.Type == LT_ImportStatement)
+  // Preprocessor directives get indented before the hash only if specified
+  if (Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
+      (Line.Type == LT_PreprocessorDirective ||
+       Line.Type == LT_ImportStatement))
     Indent = 0;
 
   Whitespaces->replaceWhitespace(RootToken, Newlines, Indent, Indent,
                                  Line.InPPDirective &&
                                      !RootToken.HasUnescapedNewline);
 }
 
 unsigned

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

nextToken();
     Line->Level -= SkipIndent ? 0 : 1;
   }
   nextToken();
 }
 
 void UnwrappedLineParser::parsePPDirective() {
   assert(FormatTok->Tok.is(tok::hash) && "'#' expected");
   ScopedMacroState MacroState(*Line, Tokens, FormatTok);
+
   nextToken();
 
   if (!FormatTok->Tok.getIdentifierInfo()) {
     parsePPUnknown();
     return;
   }
 
   switch (FormatTok->Tok.getIdentifierInfo()->getPPKeywordID()) {
void UnwrappedLineParser::parsePPDefine() {
   }
 
   nextToken();
   if (FormatTok->Tok.getKind() == tok::l_paren &&
       FormatTok->WhitespaceRange.getBegin() ==
           FormatTok->WhitespaceRange.getEnd()) {
     parseParens();
   }
-  if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash)
+  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None)
     Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
   ++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)
+  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None)
     Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
 }
 
 // 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) {
while (!Line->InPPDirective && FormatTok->Tok.is(tok::hash) &&
       ScopedLineState BlockState(*this, SwitchToPreprocessorLines);
       assert((LevelDifference >= 0 ||
               static_cast<unsigned>(-LevelDifference) <= Line->Level) &&
              "LevelDifference makes Line->Level negative");
       Line->Level += LevelDifference;
       // Comments stored before the preprocessor directive need to be output
       // before the preprocessor directive, at the same level as the
       // preprocessor directive, as we consider them to apply to the directive.
+      if (Style.IndentPPDirectives == FormatStyle::PPDIS_BeforeHash &&
+          PPBranchLevel > 0)
+        Line->Level += PPBranchLevel;
       flushComments(isOnNewLine(*FormatTok));
       parsePPDirective();
     }
     while (FormatTok->Type == TT_ConflictStart ||
            FormatTok->Type == TT_ConflictEnd ||
            FormatTok->Type == TT_ConflictAlternative) {
       if (FormatTok->Type == TT_ConflictStart) {
         conditionalCompilationStart(/*Unreachable=*/false);

cfe/trunk/unittests/Format/FormatTest.cpp

const char *ToFormat = ""
                            "// Code. Separated by blank line.\n"
                            "\n"
                            "#define B 0\n"
                            "   // Code. Not aligned with #\n"
                            "#define C 0\n";
     EXPECT_EQ(Expected, format(ToFormat, Style));
     EXPECT_EQ(Expected, format(Expected, Style));
   }
-  // Test with tabs.
-  Style.UseTab = FormatStyle::UT_Always;
-  Style.IndentWidth = 8;
-  Style.TabWidth = 8;
+  // Test AfterHash with tabs.
+  {
+    FormatStyle Tabbed = Style;
+    Tabbed.UseTab = FormatStyle::UT_Always;
+    Tabbed.IndentWidth = 8;
+    Tabbed.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);
+                 Tabbed);
+  }
 
   // Regression test: Multiline-macro inside include guards.
   verifyFormat("#ifndef HEADER_H\n"
                "#define HEADER_H\n"
                "#define A()        \\\n"
                "  int i;           \\\n"
                "  int j;\n"
                "#endif // HEADER_H",
                getLLVMStyleWithColumns(20));
+
+  Style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
+  // Basic before hash indent tests
+  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);
+  // Keep comments aligned with indented directives. These
+  // tests cannot use verifyFormat because messUp manipulates leading
+  // whitespace.
+  {
+    const char *Expected = "void f() {\n"
+                           "// Aligned to preprocessor.\n"
+                           "#if 1\n"
+                           "  // Aligned to code.\n"
+                           "  int a;\n"
+                           "  #if 1\n"
+                           "    // Aligned to preprocessor.\n"
+                           "    #define A 0\n"
+                           "  // Aligned to code.\n"
+                           "  int b;\n"
+                           "  #endif\n"
+                           "#endif\n"
+                           "}";
+    const char *ToFormat = "void f() {\n"
+                           "// Aligned to preprocessor.\n"
+                           "#if 1\n"
+                           "// Aligned to code.\n"
+                           "int a;\n"
+                           "#if 1\n"
+                           "// Aligned to preprocessor.\n"
+                           "#define A 0\n"
+                           "// Aligned to code.\n"
+                           "int b;\n"
+                           "#endif\n"
+                           "#endif\n"
+                           "}";
+    EXPECT_EQ(Expected, format(ToFormat, Style));
+    EXPECT_EQ(Expected, format(Expected, Style));
+  }
+  {
+    const char *Expected = "void f() {\n"
+                           "/* Aligned to preprocessor. */\n"
+                           "#if 1\n"
+                           "  /* Aligned to code. */\n"
+                           "  int a;\n"
+                           "  #if 1\n"
+                           "    /* Aligned to preprocessor. */\n"
+                           "    #define A 0\n"
+                           "  /* Aligned to code. */\n"
+                           "  int b;\n"
+                           "  #endif\n"
+                           "#endif\n"
+                           "}";
+    const char *ToFormat = "void f() {\n"
+                           "/* Aligned to preprocessor. */\n"
+                           "#if 1\n"
+                           "/* Aligned to code. */\n"
+                           "int a;\n"
+                           "#if 1\n"
+                           "/* Aligned to preprocessor. */\n"
+                           "#define A 0\n"
+                           "/* Aligned to code. */\n"
+                           "int b;\n"
+                           "#endif\n"
+                           "#endif\n"
+                           "}";
+    EXPECT_EQ(Expected, format(ToFormat, Style));
+    EXPECT_EQ(Expected, format(Expected, Style));
+  }
+
+  // Test single comment before preprocessor
+  verifyFormat("// Comment\n"
+               "\n"
+               "#if 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;",