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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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)));


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1280
+FormatToken *PragmaType = State.Line->First->Next->Next;
+if (PragmaType && PragmaType->TokenText.equals("omp"))
+  return CurrentState.Indent + Style.ContinuationIndentWidth;

MyDeveloperDay wrote:
> can you add a test that covers this?
There is already a test for the `omp` case and this patch added a new one for 
the non-omp case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1280
+FormatToken *PragmaType = State.Line->First->Next->Next;
+if (PragmaType && PragmaType->TokenText.equals("omp"))
+  return CurrentState.Indent + Style.ContinuationIndentWidth;

can you add a test that covers this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG466b4327f8fc: [clang-format] Only add pragma continuation 
indentation for omp clauses (authored by jhuber6).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -20560,6 +20560,21 @@
 "(including parentheses).",
 format("#pragmamark   Any non-hyphenated or hyphenated string "
"(including parentheses)."));
+
+  EXPECT_EQ("#pragma mark Any non-hyphenated or hyphenated string "
+"(including parentheses).",
+format("#pragmamark   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) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1273,8 +1273,13 @@
 return ContinuationIndent;
   }
 
-  if (State.Line->InPragmaDirective)
-return CurrentState.Indent + Style.ContinuationIndentWidth;
+  // 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];


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -20560,6 +20560,21 @@
 "(including parentheses).",
 format("#pragmamark   Any non-hyphenated or hyphenated string "
"(including parentheses)."));
+
+  EXPECT_EQ("#pragma mark Any non-hyphenated or hyphenated string "
+"(including parentheses).",
+format("#pragmamark   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) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1273,8 +1273,13 @@
 return ContinuationIndent;
   }
 
-  if (State.Line->InPragmaDirective)
-return CurrentState.Indent + Style.ContinuationIndentWidth;
+  // 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];
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 updated this revision to Diff 500872.
jhuber6 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -20560,6 +20560,21 @@
 "(including parentheses).",
 format("#pragmamark   Any non-hyphenated or hyphenated string "
"(including parentheses)."));
+
+  EXPECT_EQ("#pragma mark Any non-hyphenated or hyphenated string "
+"(including parentheses).",
+format("#pragmamark   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) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1273,8 +1273,13 @@
 return ContinuationIndent;
   }
 
-  if (State.Line->InPragmaDirective)
-return CurrentState.Indent + Style.ContinuationIndentWidth;
+  // 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];


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -20560,6 +20560,21 @@
 "(including parentheses).",
 format("#pragmamark   Any non-hyphenated or hyphenated string "
"(including parentheses)."));
+
+  EXPECT_EQ("#pragma mark Any non-hyphenated or hyphenated string "
+"(including parentheses).",
+format("#pragmamark   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) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1273,8 +1273,13 @@
 return ContinuationIndent;
   }
 
-  if (State.Line->InPragmaDirective)
-return CurrentState.Indent + Style.ContinuationIndentWidth;
+  // 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];
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1279
+  if (State.Line->InPragmaDirective) {
+FormatToken *PragmaType = State.Line->First->Next->Next;
+if (PragmaType && PragmaType->TokenText.equals("omp"))

HazardyKnusperkeks wrote:
> Do we know that the first `Next` is never null?
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
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1279
+  if (State.Line->InPragmaDirective) {
+FormatToken *PragmaType = State.Line->First->Next->Next;
+if (PragmaType && PragmaType->TokenText.equals("omp"))

Do we know that the first `Next` is never null?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

In D144884#4155730 , @jdoerfert wrote:

> I'm assuming they have tests?

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I'm assuming they have tests?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144884/new/

https://reviews.llvm.org/D144884

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 created this revision.
jhuber6 added reviewers: owenpan, MyDeveloperDay, HazardyKnusperkeks, hans.
Herald added a project: All.
jhuber6 requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144884

Files:
  clang/lib/Format/ContinuationIndenter.cpp


Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1273,8 +1273,13 @@
 return ContinuationIndent;
   }
 
-  if (State.Line->InPragmaDirective)
-return CurrentState.Indent + Style.ContinuationIndentWidth;
+  // 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];


Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1273,8 +1273,13 @@
 return ContinuationIndent;
   }
 
-  if (State.Line->InPragmaDirective)
-return CurrentState.Indent + Style.ContinuationIndentWidth;
+  // 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];
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits