[clang-format] Only add pragma continuation indentation for 'omp' clauses

[jhuber6]

Summary

The patch in D136100 added custom handling for pragmas to assist in
formatting OpenMP clauses correctly. One of these changes added extra
indentation. This is desirable for OpenMP pragmas as they are several
complete tokens that would otherwise we on the exact same line. However,
this is not desired for the other pragmas.

This solution is extremely hacky, I'm not overly familiar with the
clang-format codebase. A better solution would probably require
actually parsing these as tokens, but I just wanted to propose a
solution.

Fixes https://github.com/llvm/llvm-project/issues/59473

[jdoerfert]

I'm assuming they have tests?

[jhuber6]

In D144884#4155730, @jdoerfert wrote:

I'm assuming they have tests?

I could add a test for the comment case in the bug report.

[HazardyKnusperkeks]

In D144884#4155739, @jhuber6 wrote:

In D144884#4155730, @jdoerfert wrote:

I'm assuming they have tests?

I could add a test for the comment case in the bug report.

Yes please.

[HazardyKnusperkeks]

clang/lib/Format/ContinuationIndenter.cpp

1279

Do we know that the first Next is never null?

[jhuber6]

clang/lib/Format/ContinuationIndenter.cpp

1279

The line should only have InPragmaDirective if it found pragma, so it should look something like this if you go through the tokens. I checked the final Next because someone could do #pragma.

#
pragma
comment

[jhuber6]

Add test for case in https://github.com/llvm/llvm-project/issues/59473

[MyDeveloperDay]

clang/lib/Format/ContinuationIndenter.cpp

1280

can you add a test that covers this?

[jhuber6]

clang/lib/Format/ContinuationIndenter.cpp

1280

There is already a test for the omp case and this patch added a new one for the non-omp case.

[MyDeveloperDay]

without this change what does this look like?

EXPECT_EQ(
    "#pragma omp target       \\\n"
    "    reduction(+ : var)   \\\n"
    "    map(to : A[0 : N])   \\\n"
    "    map(to : B[0 : N])   \\\n"
    "    map(from : C[0 : N]) \\\n"
    "    firstprivate(i)      \\\n"
    "    firstprivate(j)      \\\n"
    "    firstprivate(k)",
    format(
        "#pragma omp target reduction(+:var) map(to:A[0:N]) map(to:B[0:N]) "
        "map(from:C[0:N]) firstprivate(i) firstprivate(j) firstprivate(k)",
        getLLVMStyleWithColumns(26)));

[jhuber6]

In D144884#4165192, @MyDeveloperDay wrote:

without this change what does this look like?

EXPECT_EQ(
    "#pragma omp target       \\\n"
    "    reduction(+ : var)   \\\n"
    "    map(to : A[0 : N])   \\\n"
    "    map(to : B[0 : N])   \\\n"
    "    map(from : C[0 : N]) \\\n"
    "    firstprivate(i)      \\\n"
    "    firstprivate(j)      \\\n"
    "    firstprivate(k)",
    format(
        "#pragma omp target reduction(+:var) map(to:A[0:N]) map(to:B[0:N]) "
        "map(from:C[0:N]) firstprivate(i) firstprivate(j) firstprivate(k)",
        getLLVMStyleWithColumns(26)));

Like this without the code in the previous patch

#pragma omp target reduction( \
    + : var) map(to : A       \
                     [0 : N]) \
    map(to : B[0 : N]) map(   \
        from : C[0 : N])      \
        firstprivate(i)       \
            firstprivate(     \
                j)            \
                firstprivate( \
                    k)

Like this if we just return the current indent without any extra

#pragma omp target   \\
reduction(+ : var)   \\
map(to : A[0 : N])   \\
map(to : B[0 : N])   \\
map(from : C[0 : N]) \\
firstprivate(i)      \\
firstprivate(j)      \\
firstprivate(k)

clang/lib/Format/ContinuationIndenter.cpp

if (CurrentState.StartOfArraySubscripts != 0) {
       return CurrentState.StartOfArraySubscripts;
     } else if (Style.isCSharp()) { // C# allows `["key"] = value` inside object
                                    // initializers.
       return CurrentState.Indent;
     }
     return ContinuationIndent;
   }
 
-  if (State.Line->InPragmaDirective)
+  // OpenMP clauses want to get additional indentation when they are pushed onto
+  // the next line.
+  if (State.Line->InPragmaDirective) {
+    FormatToken *PragmaType = State.Line->First->Next->Next;
+    if (PragmaType && PragmaType->TokenText.equals("omp"))
       return CurrentState.Indent + Style.ContinuationIndentWidth;
+  }
 
   // This ensure that we correctly format ObjC methods calls without inputs,
   // i.e. where the last element isn't selector like: [callee method];
   if (NextNonComment->is(tok::identifier) && NextNonComment->FakeRParens == 0 &&
       NextNonComment->Next && NextNonComment->Next->is(TT_ObjCMethodExpr)) {
     return CurrentState.Indent;
   }
 

clang/unittests/Format/FormatTest.cpp

 TEST_F(FormatTest, UnderstandsPragmas) {
   verifyFormat("#pragma omp reduction(| : var)");
   verifyFormat("#pragma omp reduction(+ : var)");
 
   EXPECT_EQ("#pragma mark Any non-hyphenated or hyphenated string "
             "(including parentheses).",
             format("#pragma    mark   Any non-hyphenated or hyphenated string "
                    "(including parentheses)."));
+
+  EXPECT_EQ("#pragma mark Any non-hyphenated or hyphenated string "
+            "(including parentheses).",
+            format("#pragma    mark   Any non-hyphenated or hyphenated string "
+                   "(including parentheses)."));
+
+  EXPECT_EQ(
+      "#pragma comment(linker,    \\\n"
+      "                \"argument\" \\\n"
+      "                \"argument\"",
+      format("#pragma comment(linker,      \\\n"
+             "                 \"argument\" \\\n"
+             "                 \"argument\"",
+             getStyleWithColumns(
+                 getChromiumStyle(FormatStyle::LanguageKind::LK_Cpp), 32)));
 }
 
 TEST_F(FormatTest, UnderstandsPragmaOmpTarget) {
   verifyFormat("#pragma omp target map(to : var)");
   verifyFormat("#pragma omp target map(to : var[ : N])");
   verifyFormat("#pragma omp target map(to : var[0 : N])");
   verifyFormat("#pragma omp target map(always, to : var[0 : N])");