[clang-format] Add IndentPragma style to eliminate common clang-format off scenario

[MyDeveloperDay]

Summary

A quick search of github.com, shows one common scenario for excessive use of //clang-format off/on is the indentation of #pragma's, especially around the areas of loop optimization or OpenMP

This revision aims to help that by introducing an IndentPragmas style, the aim of which is to keep the pragma at the current level of scope

    for (int i = 0; i < 5; i++) {
// clang-format off
        #pragma HLS UNROLL
        // clang-format on
        for (int j = 0; j < 5; j++) {
// clang-format off
            #pragma HLS UNROLL
            // clang-format on
     ....

can become

for (int i = 0; i < 5; i++) {
    #pragma HLS UNROLL
    for (int j = 0; j < 5; j++) {
        #pragma HLS UNROLL
    ....

This revision also support working alongside the IndentPPDirective of BeforeHash and AfterHash (see unit tests for examples)

[njames93]

Should this be controlled on a case by case basis, maybe control indentation using a regex over the pragma arguments, WDYT?

clang/docs/ClangFormatStyleOptions.rst

1923–1924

Please add full stops at the end of these sentences.

1928

clang/include/clang/Format/Format.h

1533–1534

Please add full stops at the end of these sentences.

1536

clang/lib/Format/ContinuationIndenter.cpp

594

Full stop.

595–597

Can elide these braces.

clang/lib/Format/UnwrappedLineFormatter.cpp

1248

Capital letter at the start of comments.

[MyDeveloperDay]

Address review comments

[MyDeveloperDay]

Should this be controlled on a case by case basis, maybe control indentation using a regex over the pragma arguments, WDYT?

Most uses of pragmas seem to be at the 0'th level of scrope #pragma once etc

Most other pragmas are probably with the code (like comments) as such it seems we should add an option to keep it with the scope of the code, I think a regex is overkill, but we could add it later if deemed necessary.

[curdeius]

This LGTM in general. I just have a doubt.
You've added messUp parameter to verifyFormat, because, IIUC, pragmas wouldn't be at the desired scope level indentation otherwise.
Shouldn't clang-format find out what the correct indentation level for pragmas should be instead of keeping the current indentation (when IndentPragmas: true)?
Correct me if my understanding is wrong.
Anyway, my point is that turning off messUp seems like a bad idea and may hide some problems, nope?

What would be the output of:

void foo() {
#pragma omp
  for (...) {
#pragma omp
    for (...) {
    }
  }
}

Do the #pragmas get intended to the level of corresponding fors or not?

[MyDeveloperDay]

In D92753#2436852, @curdeius wrote:

You've added messUp parameter to verifyFormat, because, IIUC, pragmas wouldn't be at the desired scope level indentation otherwise.

messUp was pulling the line below onto the pragma line, which was then unable to determine when if it should break it

i.e.

#pragma omp simd
for (int k = 0; k < 10; k++) {

became

#pragma omp simd for (int k = 0; k < 10; k++) {

I'm not sure why this occurred, but I assume there is no real token parsing of an #pragam line.

[MyDeveloperDay]

Add for(...) test case

[curdeius]

Maybe it would be judicious to add a FIXME to test::messUp for the pragmas? Or fix it in this patch?

clang/unittests/Format/FormatTest.cpp

17647–17657

Actually I'd rather see a test were you mess up spaces manually (adding or removing spaces on pragma lines).
So instead of verifyFormat(formattedCode), I'd see EXPECT_EQ(formattedCode, format(lightly-messed-up-code, Style).

[MyDeveloperDay]

Allow the tests to messUp() the code, turns out its some sort of interaction ONLY with the BeforeHash case (last test)

[curdeius]

LGTM with a single comment on the test.

clang/unittests/Format/FormatTest.cpp

17685–17693

Last nit, I'd mess up spaces like suggested here.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

17685–17693

This is BeforeHash not AfterHash, I think the test is correct (as it passes)

[curdeius]

clang/unittests/Format/FormatTest.cpp

17685–17693

Ok. My thought was that we were not really checking that this patch will correctly reformat pragmas that are not yet on correct columns, but after your change to remove messUp=false, we do test it.
Again, LGTM! And thank you for the patch (and the patience) :).

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

17685–17693

Its never a problem @curdeius, I'm always happy to wait for your feedback as its considered and insightful, your comment made be go back and double check and that can never be a bad thing. Thanks for the accept.

[RatTac]

This is a great feature. However I found two issues:

First issue is with combination of IndentPPDirectives

// IndentPPDirectives: BeforeHash
// IndentPragmas: true

#if defined(WIN32)
	#define TEST // Correctly indented because IndentPPDirectives: BeforeHash
#endif

#if defined(WIN32)
#pragma warning(disable : 4005) // NOT indented even though IndentPPDirectives: BeforeHash
#endif

Second issue is with combination of lambda function:

inline void ompExecuteParallel(unsigned int endIndex, const std::function<void(unsigned int i)>& computeFunction, unsigned int startIdx)
{
	#pragma omp parallel for
	for(int i = (int)startIdx; i < (int)endIndex; i++)
		computeFunction(i);
}

int main()
{
	#pragma omp parallel for
	for(int i = 0; i < 10; i++)
	{
		// Correctly indented because IndentPragmas: true
		#pragma openmp critical
		{
			std::cout << i << endl;
		}
	}

	ompExecuteParallel(10, [&](unsigned int i) {
	#pragma openmp critical // Incorrectly indented even though IndentPragmas: true
		{
			std::cout << i << endl;
		}
	});

	return 0;
}

Hope this is the right place to raise concerns...

Regards
Tobias

[MyDeveloperDay]

Sorry but due to the following issue raised by @RatTac , I'm reverting this prior to the LLVM 12 branch out so I can work on it further.

I also think this would impact #pragma's in any ifdef code which I envisage would be significant.

#if defined(WIN32)
#pragma warning(disable : 4005) // NOT indented even though IndentPPDirectives: BeforeHash
#endif

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
        #if FOO
          #if BAR
            #include <foo>
          #endif
        #endif
 
 
 
+**IndentPragmas** (``bool``)
+  Indent pragmas
+
+  When ``false``, pragmas are flushed left or follow IndentPPDirectives.
+  When ``true``, pragmas are indented to the current scope level.
+
+  .. code-block:: c++
+
+    false:                                  true:
+    #pragma once                   vs       #pragma once
+    void foo() {                            void foo() {
+    #pragma omp simd                          #pragma omp simd
+      for (int i=0;i<10;i++) {                for (int i=0;i<10;i++) {
+    #pragma omp simd                            #pragma omp simd
+        for (int i=0;i<10;i++) {                for (int i=0;i<10;i++) {
+        }                                       }
+    #if 1                                   #if 1
+    #pragma omp simd                            #pragma omp simd
+        for (int i=0;i<10;i++) {                for (int i=0;i<10;i++) {
+        }                                       }
+    #endif                                  #endif
+      }                                       }
+    }                                       }
+
 **IndentRequires** (``bool``)
   Indent the requires clause in a template
 
   .. code-block:: c++
 
      true:
      template <typename It>
        requires Iterator<It>

clang/include/clang/Format/Format.h

   ///        bar();                                 bar();
   ///      }                                      }
   ///    label2:                                label2:
   ///      return 1;                              return 1;
   ///    }                                      }
   /// \endcode
   bool IndentGotoLabels;
 
+  /// Indent pragmas
+  ///
+  /// When ``false``, pragmas are flushed left or follow IndentPPDirectives.
+  /// When ``true``, pragmas are indented to the current scope level.
+  /// \code
+  ///   false:                                  true:
+  ///   #pragma once                   vs       #pragma once
+  ///   void foo() {                            void foo() {
+  ///   #pragma omp simd                          #pragma omp simd
+  ///     for (int i=0;i<10;i++) {                for (int i=0;i<10;i++) {
+  ///   #pragma omp simd                            #pragma omp simd
+  ///       for (int i=0;i<10;i++) {                for (int i=0;i<10;i++) {
+  ///       }                                       }
+  ///   #if 1                                   #if 1
+  ///   #pragma omp simd                            #pragma omp simd
+  ///       for (int i=0;i<10;i++) {                for (int i=0;i<10;i++) {
+  ///       }                                       }
+  ///   #endif                                  #endif
+  ///     }                                       }
+  ///   }                                       }
+  /// \endcode
+  bool IndentPragmas;
+
   /// Options for indenting preprocessor directives.
   enum PPDirectiveIndentStyle {
     /// Does not indent any directives.
     /// \code
     ///    #if FOO
     ///    #if BAR
     ///    #include <foo>
     ///    #endif
return AccessModifierOffset == R.AccessModifierOffset &&
            IncludeStyle.IncludeCategories == R.IncludeStyle.IncludeCategories &&
            IncludeStyle.IncludeIsMainRegex ==
                R.IncludeStyle.IncludeIsMainRegex &&
            IncludeStyle.IncludeIsMainSourceRegex ==
                R.IncludeStyle.IncludeIsMainSourceRegex &&
            IndentCaseLabels == R.IndentCaseLabels &&
            IndentCaseBlocks == R.IndentCaseBlocks &&
            IndentGotoLabels == R.IndentGotoLabels &&
+           IndentPragmas == R.IndentPragmas &&
            IndentPPDirectives == R.IndentPPDirectives &&
            IndentExternBlock == R.IndentExternBlock &&
            IndentRequires == R.IndentRequires && IndentWidth == R.IndentWidth &&
            Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
            JavaImportGroups == R.JavaImportGroups &&
            JavaScriptQuotes == R.JavaScriptQuotes &&
            JavaScriptWrapImports == R.JavaScriptWrapImports &&

clang/lib/Format/ContinuationIndenter.cpp

void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
   // 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;
 
+    bool isPragmaLine =
+        State.Line->First->startsSequence(tok::hash, tok::pp_pragma);
+    // If indenting pragmas remove the extra space for the #.
+    if (Style.IndentPragmas && isPragmaLine)
+      Spaces--;
+
     // 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;
   }
 

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("IncludeBlocks", Style.IncludeStyle.IncludeBlocks);
     IO.mapOptional("IncludeCategories", Style.IncludeStyle.IncludeCategories);
     IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
     IO.mapOptional("IncludeIsMainSourceRegex",
                    Style.IncludeStyle.IncludeIsMainSourceRegex);
     IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
     IO.mapOptional("IndentCaseBlocks", Style.IndentCaseBlocks);
     IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
+    IO.mapOptional("IndentPragmas", Style.IndentPragmas);
     IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
     IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
     IO.mapOptional("IndentRequires", Style.IndentRequires);
     IO.mapOptional("IndentWidth", Style.IndentWidth);
     IO.mapOptional("IndentWrappedFunctionNames",
                    Style.IndentWrappedFunctionNames);
     IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
     IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
LLVMStyle.IncludeStyle.IncludeCategories = {
       {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
       {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false},
       {".*", 1, 0, false}};
   LLVMStyle.IncludeStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentCaseBlocks = false;
   LLVMStyle.IndentGotoLabels = true;
+  LLVMStyle.IndentPragmas = false;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentRequires = false;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
   LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
   LLVMStyle.TabWidth = 8;

clang/lib/Format/UnwrappedLineFormatter.cpp

void UnwrappedLineFormatter::formatFirstToken(
   // If in Whitemsmiths mode, indent start and end of blocks
   if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
     if (RootToken.isOneOf(tok::l_brace, tok::r_brace, tok::kw_case,
                           tok::kw_default))
       Indent += Style.IndentWidth;
   }
 
   // Preprocessor directives get indented before the hash only if specified
-  if (Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
-      (Line.Type == LT_PreprocessorDirective ||
-       Line.Type == LT_ImportStatement))
+  if (Line.Type == LT_PreprocessorDirective ||
+      Line.Type == LT_ImportStatement) {
+    switch (Style.IndentPPDirectives) {
+    case FormatStyle::PPDIS_AfterHash:
       Indent = 0;
+      break;
+    case FormatStyle::PPDIS_None:
+    case FormatStyle::PPDIS_BeforeHash: {
+      // If we want to indent pragmas.
+      bool isPragmaLine = RootToken.startsSequence(tok::hash, tok::pp_pragma);
+      if (!Style.IndentPragmas && isPragmaLine)
+        Indent = 0;
+    } break;
+    }
+  }
 
   Whitespaces->replaceWhitespace(RootToken, Newlines, Indent, Indent,
                                  /*IsAligned=*/false,
                                  Line.InPPDirective &&
                                      !RootToken.HasUnescapedNewline);
 }
 
 unsigned

clang/lib/Format/UnwrappedLineParser.h

 
 private:
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
   void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
                   bool MunchSemi = true);
   void parseChildBlock();
-  void parsePPDirective();
+  void parsePPDirective(unsigned Level);
   void parsePPDefine();
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
   void parsePPUnknown();
   void readTokenWithJavaScriptASI();
   void parseStructuralElement();

clang/lib/Format/UnwrappedLineParser.cpp

nextToken();
     Line->Level += SkipIndent ? 0 : 1;
     parseLevel(/*HasOpeningBrace=*/true);
     flushComments(isOnNewLine(*FormatTok));
     Line->Level -= SkipIndent ? 0 : 1;
   }
   nextToken();
 }
 
-void UnwrappedLineParser::parsePPDirective() {
+void UnwrappedLineParser::parsePPDirective(unsigned Level) {
   assert(FormatTok->Tok.is(tok::hash) && "'#' expected");
   ScopedMacroState MacroState(*Line, Tokens, FormatTok);
 
   nextToken();
 
   if (!FormatTok->Tok.getIdentifierInfo()) {
     parsePPUnknown();
     return;
case tok::pp_else:
     parsePPElse();
     break;
   case tok::pp_elif:
     parsePPElIf();
     break;
   case tok::pp_endif:
     parsePPEndIf();
     break;
+  case tok::pp_pragma: {
+    bool IndentPPDirectives =
+        Style.IndentPPDirectives != FormatStyle::PPDIS_None;
+    unsigned CurrentLevel = Line->Level;
+    Line->Level =
+        Style.IndentPragmas
+            ? (IndentPPDirectives ? (Level - (PPBranchLevel + 1)) : Level)
+            : CurrentLevel;
+    parsePPUnknown();
+    Line->Level = CurrentLevel;
+  } break;
   default:
     parsePPUnknown();
     break;
   }
 }
 
 void UnwrappedLineParser::conditionalCompilationCondition(bool Unreachable) {
   size_t Line = CurrentLines->size();
while (!Line->InPPDirective && FormatTok->Tok.is(tok::hash) &&
       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();
+      parsePPDirective(Line->Level);
     }
     while (FormatTok->getType() == TT_ConflictStart ||
            FormatTok->getType() == TT_ConflictEnd ||
            FormatTok->getType() == TT_ConflictAlternative) {
       if (FormatTok->getType() == TT_ConflictStart) {
         conditionalCompilationStart(/*Unreachable=*/false);
       } else if (FormatTok->getType() == TT_ConflictAlternative) {
         conditionalCompilationAlternative();

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DeriveLineEnding);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
+  CHECK_PARSE_BOOL(IndentPragmas);
   CHECK_PARSE_BOOL(IndentRequires);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
   CHECK_PARSE_BOOL(SortIncludes);
TEST_F(FormatTest, ConceptsAndRequires) {
   verifyFormat("template <typename T> concept Tree = true;", Style);
 
   Style.IndentRequires = false;
   verifyFormat("template <std::semiregular F, std::semiregular... Args>\n"
                "requires (std::invocable<F, std::invoke_result_t<Args>...>) "
                "struct constant;",
                Style);
 }
+
+TEST_F(FormatTest, IndentPragmas) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentPPDirectives = FormatStyle::PPDIS_None;
+
+  Style.IndentPragmas = false;
+  verifyFormat("#pragma once", Style);
+  verifyFormat("#pragma omp simd\n"
+               "for (int i = 0; i < 10; i++) {\n"
+               "}",
+               Style);
+  verifyFormat("void foo() {\n"
+               "#pragma omp simd\n"
+               "  for (int i = 0; i < 10; i++) {\n"
+               "  }\n"
+               "}",
+               Style);
+  verifyFormat("void foo() {\n"
+               "// outer loop\n"
+               "#pragma omp simd\n"
+               "  for (int k = 0; k < 10; k++) {\n"
+               "// inner loop\n"
+               "#pragma omp simd\n"
+               "    for (int j = 0; j < 10; j++) {\n"
+               "    }\n"
+               "  }\n"
+               "}",
+               Style);
+
+  verifyFormat("void foo() {\n"
+               "// outer loop\n"
+               "#if 1\n"
+               "#pragma omp simd\n"
+               "  for (int k = 0; k < 10; k++) {\n"
+               "// inner loop\n"
+               "#pragma omp simd\n"
+               "    for (int j = 0; j < 10; j++) {\n"
+               "    }\n"
+               "  }\n"
+               "#endif\n"
+               "}",
+               Style);
+
+  Style.IndentPragmas = true;
+  verifyFormat("#pragma once", Style);
+  verifyFormat("#pragma omp simd\n"
+               "for (int i = 0; i < 10; i++) {\n"
+               "}",
+               Style);
+  verifyFormat("void foo() {\n"
+               "  #pragma omp simd\n"
+               "  for (int i = 0; i < 10; i++) {\n"
+               "  }\n"
+               "}",
+               Style);
+  verifyFormat("void foo() {\n"
+               "  #pragma omp simd\n"
+               "  for (int i = 0; i < 10; i++) {\n"
+               "    #pragma omp simd\n"
+               "    for (int j = 0; j < 10; j++) {\n"
+               "    }\n"
+               "  }\n"
+               "}",
+               Style);
+
+  verifyFormat("void foo() {\n"
+               "  #pragma omp simd\n"
+               "  for (...) {\n"
+               "    #pragma omp simd\n"
+               "    for (...) {\n"
+               "    }\n"
+               "  }\n"
+               "}",
+               Style);
+
+  Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+
+  verifyFormat("void foo() {\n"
+               "# pragma omp simd\n"
+               "  for (int i = 0; i < 10; i++) {\n"
+               "#   pragma omp simd\n"
+               "    for (int j = 0; j < 10; j++) {\n"
+               "    }\n"
+               "  }\n"
+               "}",
+               Style);
+
+  verifyFormat("void foo() {\n"
+               "#if 1\n"
+               "# pragma omp simd\n"
+               "  for (int k = 0; k < 10; k++) {\n"
+               "#   pragma omp simd\n"
+               "    for (int j = 0; j < 10; j++) {\n"
+               "    }\n"
+               "  }\n"
+               "#endif\n"
+               "}",
+               Style);
+
+  Style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
+  EXPECT_EQ("void foo() {\n"
+            "#if 1\n"
+            "  #pragma omp simd\n"
+            "  for (int k = 0; k < 10; k++) {\n"
+            "    #pragma omp simd\n"
+            "    for (int j = 0; j < 10; j++) {\n"
+            "    }\n"
+            "  }\n"
+            "#endif\n"
+            "}",
+            format("void foo() {\n"
+                   "#if 1\n"
+                   "  #pragma omp simd\n"
+                   "  for (int k = 0; k < 10; k++) {\n"
+                   "    #pragma omp simd\n"
+                   "    for (int j = 0; j < 10; j++) {\n"
+                   "    }\n"
+                   "  }\n"
+                   "#endif\n"
+                   "}",
+                   Style));
+}
+
 } // namespace
 } // namespace format
 } // namespace clang