[PATCH] D151145: Add disabled unittest reproducing TextProto formatting issue.

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

HazardyKnusperkeks added a comment.

> In D151145#4365580 , 
> @HazardyKnusperkeks wrote:
>
>> As stated on Github I don't think we have a bug.
>
> Are you sure, I'm pretty sure this is a bug? Can you elaborate on the 
> discussion on github. I wouldn't want to fix something that isn't broken. If 
> this is indeed expected maybe I can update the docs.

Everything good, seems to be a bug.




Comment at: clang/unittests/Format/FormatTestRawStrings.cpp:177
+format(R"test(
+t = R"pb(item:1)pb";)test",
+   getRawStringPbStyleSeparateSection()));

Icantjuddle wrote:
> HazardyKnusperkeks wrote:
> > EXPECT_EQ
> I used the function instead of the macro to conform to the convention of 
> every other test in the file. 
> 
> The function provides its own justification.
> ```
>   // Gcc 4.8 doesn't support raw string literals in macros, which breaks some
>   // build bots. We use this function instead.
>   void expect_eq(const std::string Expected, const std::string Actual) {
> ```
And every other clang-format test file (at least I worked on) uses the macro.

Also I don't think gcc 4.8 will be able to compile LLVM anymore, but I may be 
wrong on that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151145

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

[PATCH] D151145: Add disabled unittest reproducing TextProto formatting issue.

[Ben J via Phabricator via cfe-commits]

Icantjuddle added a comment.

In D151145#4369406 , @MyDeveloperDay 
wrote:

> We don't normally land broken tests, even if they are disabled, its better 
> for us if we get a fix at the same time ;-)



In D151145#4367495 , @krasimir wrote:

> ...
> @Icantjuddle would you like to update this patch to fix the issue and enable 
> your newly added tests?

Sounds like we don't really want this without a real fix; I'll give it a shot, 
hopefully won't be too bad. Thanks for the code pointer.

In D151145#4365580 , 
@HazardyKnusperkeks wrote:

> As stated on Github I don't think we have a bug.

Are you sure, I'm pretty sure this is a bug? Can you elaborate on the 
discussion on github. I wouldn't want to fix something that isn't broken. If 
this is indeed expected maybe I can update the docs.




Comment at: clang/unittests/Format/FormatTestRawStrings.cpp:92-99
 Style.RawStringFormats = {
 {
 /*Language=*/FormatStyle::LK_Cpp,
 /*Delimiters=*/{"cpp"},
 /*EnclosingFunctions=*/{},
 /*CanonicalDelimiter=*/"",
 BasedOnStyle,

HazardyKnusperkeks wrote:
> Couldn't you just set the style like you want, instead of parsing yaml?
Given that the problem seems to be with how the configs are parsed/processed, I 
wanted the repro to be most faithful to the user facing issue.



Comment at: clang/unittests/Format/FormatTestRawStrings.cpp:177
+format(R"test(
+t = R"pb(item:1)pb";)test",
+   getRawStringPbStyleSeparateSection()));

HazardyKnusperkeks wrote:
> EXPECT_EQ
I used the function instead of the macro to conform to the convention of every 
other test in the file. 

The function provides its own justification.
```
  // Gcc 4.8 doesn't support raw string literals in macros, which breaks some
  // build bots. We use this function instead.
  void expect_eq(const std::string Expected, const std::string Actual) {
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151145

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

[PATCH] D151145: Add disabled unittest reproducing TextProto formatting issue.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

We don't normally land broken tests, even if they are disabled, its better for 
us if we get a fix at the same time ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151145

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

[PATCH] D151145: Add disabled unittest reproducing TextProto formatting issue.

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

Thank you! That's a bug in the raw string format manager.
This code effectively first looks for a matching top-level style, and if that's 
not found, then it tries to derive one via the RawFormat's BasedOnStyle:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Format/ContinuationIndenter.cpp#L191-L201

This doesn't match the documentation of the BasedOnStyle field, which indicates 
that, if specified, it should take precedence:
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Format/Format.h#L3219-L3222

@Icantjuddle would you like to update this patch to fix the issue and enable 
your newly added tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151145

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

[PATCH] D151145: Add disabled unittest reproducing TextProto formatting issue.

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

HazardyKnusperkeks added reviewers: MyDeveloperDay, owenpan, rymiel, 
HazardyKnusperkeks.
HazardyKnusperkeks added a comment.

As stated on Github I don't think we have a bug.




Comment at: clang/unittests/Format/FormatTestRawStrings.cpp:79
+)format";
+llvm::MemoryBufferRef ConfigBuffer(FormatStr, "ClangFormatConfig");
+FormatStyle Style = {};

Why such a raw string, instead of just `"(...)"`?



Comment at: clang/unittests/Format/FormatTestRawStrings.cpp:79
+)format";
+llvm::MemoryBufferRef ConfigBuffer(FormatStr, "ClangFormatConfig");
+FormatStyle Style = {};

HazardyKnusperkeks wrote:
> Why such a raw string, instead of just `"(...)"`?
I think `StringRef` would be the correct type.



Comment at: clang/unittests/Format/FormatTestRawStrings.cpp:92-99
 Style.RawStringFormats = {
 {
 /*Language=*/FormatStyle::LK_Cpp,
 /*Delimiters=*/{"cpp"},
 /*EnclosingFunctions=*/{},
 /*CanonicalDelimiter=*/"",
 BasedOnStyle,

Couldn't you just set the style like you want, instead of parsing yaml?



Comment at: clang/unittests/Format/FormatTestRawStrings.cpp:177
+format(R"test(
+t = R"pb(item:1)pb";)test",
+   getRawStringPbStyleSeparateSection()));

EXPECT_EQ


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151145

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

[PATCH] D151145: Add disabled unittest reproducing TextProto formatting issue.

[Ben J via Phabricator via cfe-commits]

Icantjuddle removed a reviewer: craig.topper.
Icantjuddle added a comment.

Sorry @craig.topper for some reason `git blame` locally attributed much of the 
edits to you, doesn't match what I see online at github.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151145

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

[PATCH] D151145: Add disabled unittest reproducing TextProto formatting issue.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

I don't think I'm the right reviewer for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151145

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

[PATCH] D151145: Add disabled unittest reproducing TextProto formatting issue.

[Ben J via Phabricator via cfe-commits]

Icantjuddle created this revision.
Icantjuddle added a reviewer: clang-format.
Herald added a project: All.
Icantjuddle requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Github Issue 

Basically adding an empty `TextProto` section causes the formatting of 
`TextProto` raw strings to change and spaces to be added after the `: `. 
This diff adds a test that reproduces this issus.
It is currently disabled as it is failing, here is the output:

  ❯ build/tools/clang/unittests/Format/FormatTests 
--gtest_filter='FormatTestRawStrings.DISABLED*' --gtest_also_run_disabled_tests
  Note: Google Test filter = FormatTestRawStrings.DISABLED*
  [==] Running 1 test from 1 test suite.
  [--] Global test environment set-up.
  [--] 1 test from FormatTestRawStrings
  [ RUN  ] 
FormatTestRawStrings.DISABLED_FormatsCorrectlyWhenTextProtoIsInOwnSection
  
/home/bjudd/code/llvm-project/clang/unittests/Format/FormatTestRawStrings.cpp:134:
 Failure
  Expected equality of these values:
Expected
  Which is: "\nt = R\"pb(item: 1)pb\";"
Actual
  Which is: "\nt = R\"pb(item : 1)pb\";"
  With diff:
  @@ -1,2 +1,2 @@
  
  -t = R\"pb(item: 1)pb\";
  +t = R\"pb(item : 1)pb\";
  
  [  FAILED  ] 
FormatTestRawStrings.DISABLED_FormatsCorrectlyWhenTextProtoIsInOwnSection (7 ms)
  [--] 1 test from FormatTestRawStrings (7 ms total)
  
  [--] Global test environment tear-down
  [==] 1 test from 1 test suite ran. (7 ms total)
  [  PASSED  ] 0 tests.
  [  FAILED  ] 1 test, listed below:
  [  FAILED  ] 
FormatTestRawStrings.DISABLED_FormatsCorrectlyWhenTextProtoIsInOwnSection
  
   1 FAILED TEST


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151145

Files:
  clang/unittests/Format/FormatTestRawStrings.cpp


Index: clang/unittests/Format/FormatTestRawStrings.cpp
===
--- clang/unittests/Format/FormatTestRawStrings.cpp
+++ clang/unittests/Format/FormatTestRawStrings.cpp
@@ -75,6 +75,31 @@
 return Style;
   }

+  FormatStyle getRawStringPbStyleSeparateSection() {
+constexpr const char *FormatStr = R"format(---
+BasedOnStyle: LLVM
+ColumnLimit: 40
+RawStringFormats:
+  - Language: TextProto
+Delimiters:
+  - 'pb'
+BasedOnStyle: google
+---
+Language: TextProto
+# Don't set any options.
+...
+)format";
+llvm::MemoryBufferRef ConfigBuffer(FormatStr, "ClangFormatConfig");
+FormatStyle Style = {};
+Style.Language = FormatStyle::LK_Cpp;
+if (auto Err = parseConfiguration(ConfigBuffer, )) {
+  llvm::dbgs() << Err.value();
+  llvm::dbgs() << Err.message();
+  assert(Err);
+}
+return Style;
+  }
+
   FormatStyle getRawStringLLVMCppStyleBasedOn(std::string BasedOnStyle) {
 FormatStyle Style = getLLVMStyle();
 Style.RawStringFormats = {
@@ -164,6 +189,18 @@
getRawStringPbStyleWithColumns(40)));
 }

+// Currently a bug in how configs are processed:
+// See: https://github.com/llvm/llvm-project/issues/62871
+TEST_F(FormatTestRawStrings,
+   DISABLED_FormatsCorrectlyWhenTextProtoIsInOwnSection) {
+  // TextProto formatting works when in a RawString.
+  expect_eq(R"test(
+t = R"pb(item: 1)pb";)test",
+format(R"test(
+t = R"pb(item:1)pb";)test",
+   getRawStringPbStyleSeparateSection()));
+}
+
 TEST_F(FormatTestRawStrings, RespectsClangFormatOff) {
   expect_eq(R"test(
 // clang-format off


Index: clang/unittests/Format/FormatTestRawStrings.cpp
===
--- clang/unittests/Format/FormatTestRawStrings.cpp
+++ clang/unittests/Format/FormatTestRawStrings.cpp
@@ -75,6 +75,31 @@
 return Style;
   }

+  FormatStyle getRawStringPbStyleSeparateSection() {
+constexpr const char *FormatStr = R"format(---
+BasedOnStyle: LLVM
+ColumnLimit: 40
+RawStringFormats:
+  - Language: TextProto
+Delimiters:
+  - 'pb'
+BasedOnStyle: google
+---
+Language: TextProto
+# Don't set any options.
+...
+)format";
+llvm::MemoryBufferRef ConfigBuffer(FormatStr, "ClangFormatConfig");
+FormatStyle Style = {};
+Style.Language = FormatStyle::LK_Cpp;
+if (auto Err = parseConfiguration(ConfigBuffer, )) {
+  llvm::dbgs() << Err.value();
+  llvm::dbgs() << Err.message();
+  assert(Err);
+}
+return Style;
+  }
+
   FormatStyle getRawStringLLVMCppStyleBasedOn(std::string BasedOnStyle) {
 FormatStyle Style = getLLVMStyle();
 Style.RawStringFormats = {
@@ -164,6 +189,18 @@
getRawStringPbStyleWithColumns(40)));
 }

+// Currently a bug in how configs are processed:
+// See: https://github.com/llvm/llvm-project/issues/62871
+TEST_F(FormatTestRawStrings,
+   DISABLED_FormatsCorrectlyWhenTextProtoIsInOwnSection) {
+  // TextProto formatting works when in a RawString.
+  expect_eq(R"test(
+t =