[PATCH] D53482: Add clang-format stability check with FormatTests

[Vladimir Glavnyy via Phabricator via cfe-commits]

vglavnyy added a comment.

@krasimir thank you for review.
The patch code has been updated.
I hope this patch will help to start to fix this issue.
Probably will be helpful to add an optional debug flag will enable a twice-run 
checking for every run of clang-format.


https://reviews.llvm.org/D53482



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

[PATCH] D53482: Add clang-format stability check with FormatTests

[Vladimir Glavnyy via Phabricator via cfe-commits]

vglavnyy updated this revision to Diff 170663.
vglavnyy added a comment.

A twice-format problem: the format of format isn't format.
This commit surface an instability problem in clang-format at unit-tests level.
The patch adds double-checking format method for all test and adds a stub for 
tests with the detected TF-problem.


https://reviews.llvm.org/D53482

Files:
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -36,9 +36,9 @@
 SC_DoNotCheck
   };
 
-  std::string format(llvm::StringRef Code,
- const FormatStyle  = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+  std::string format_weak(llvm::StringRef Code,
+  const FormatStyle  = getLLVMStyle(),
+  StatusCheck CheckComplete = SC_ExpectComplete) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
 std::vector Ranges(1, tooling::Range(0, Code.size()));
@@ -57,6 +57,33 @@
 return *Result;
   }
 
+  // Format method with invariant check: format(Code)==format(format(Code)).
+  std::string format(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+const auto Formatted = format_weak(Code, Style, CheckComplete);
+if (CheckComplete != SC_DoNotCheck) {
+  auto SaveReplacementCount = ReplacementCount;
+  EXPECT_EQ(Formatted, format_weak(Formatted, Style, CheckComplete));
+  ReplacementCount = SaveReplacementCount;
+}
+return Formatted;
+  }
+
+  // A stub-method for tests with broken invariant: format(Code)!=format(format(Code)).
+  // Will be redundant when all tests become stable.
+  std::string format_unstable(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+const auto Formatted = format_weak(Code, Style, CheckComplete);
+if (CheckComplete != SC_DoNotCheck) {
+  auto SaveReplacementCount = ReplacementCount;
+  EXPECT_NE(Formatted, format_weak(Formatted, Style, CheckComplete));
+  ReplacementCount = SaveReplacementCount;
+}
+return Formatted;
+  }
+
   FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
 Style.ColumnLimit = ColumnLimit;
 return Style;
@@ -8147,12 +8174,14 @@
 
 
 TEST_F(FormatTest, BreaksStringLiterals) {
-  EXPECT_EQ("\"some text \"\n"
-"\"other\";",
-format("\"some text other\";", getLLVMStyleWithColumns(12)));
-  EXPECT_EQ("\"some text \"\n"
-"\"other\";",
-format("\\\n\"some text other\";", getLLVMStyleWithColumns(12)));
+  EXPECT_EQ(
+  "\"some text \"\n"
+  "\"other\";",
+  format_unstable("\"some text other\";", getLLVMStyleWithColumns(12)));
+  EXPECT_EQ(
+  "\"some text \"\n"
+  "\"other\";",
+  format_unstable("\\\n\"some text other\";", getLLVMStyleWithColumns(12)));
   EXPECT_EQ(
   "#define A  \\\n"
   "  \"some \"  \\\n"
@@ -8172,22 +8201,22 @@
 format("\"some text\"", getLLVMStyleWithColumns(11)));
   EXPECT_EQ("\"some \"\n"
 "\"text\"",
-format("\"some text\"", getLLVMStyleWithColumns(10)));
+format_unstable("\"some text\"", getLLVMStyleWithColumns(10)));
   EXPECT_EQ("\"some \"\n"
 "\"text\"",
-format("\"some text\"", getLLVMStyleWithColumns(7)));
+format_unstable("\"some text\"", getLLVMStyleWithColumns(7)));
   EXPECT_EQ("\"some\"\n"
 "\" tex\"\n"
 "\"t\"",
-format("\"some text\"", getLLVMStyleWithColumns(6)));
+format_unstable("\"some text\"", getLLVMStyleWithColumns(6)));
   EXPECT_EQ("\"some\"\n"
 "\" tex\"\n"
 "\" and\"",
-format("\"some tex and\"", getLLVMStyleWithColumns(6)));
+format_unstable("\"some tex and\"", getLLVMStyleWithColumns(6)));
   EXPECT_EQ("\"some\"\n"
 "\"/tex\"\n"
 "\"/and\"",
-format("\"some/tex/and\"", getLLVMStyleWithColumns(6)));
+format_unstable("\"some/tex/and\"", getLLVMStyleWithColumns(6)));
 
   EXPECT_EQ("variable =\n"
 "\"long string \"\n"
@@ -8237,34 +8266,32 @@
   ",\n"
   "aa(\"aaa a aaa aaa a aaa a aaa aaa aa\"));");
 
-  EXPECT_EQ("\"splitmea\"\n"
-"\"trandomp\"\n"
-"\"oint\"",
-format("\"splitmeatrandompoint\"", getLLVMStyleWithColumns(10)));
+  EXPECT_EQ(
+  "\"splitmea\"\n"
+  "\"trandomp\"\n"
+  "\"oint\"",
+  format_unstable("\"splitmeatrandompoint\"", getLLVMStyleWithColumns(10)));
 
-  EXPECT_EQ("\"split/\"\n"
-"\"pathat/\"\n"
- 

[PATCH] D53482: Add clang-format stability check with FormatTests

[Vladimir Glavnyy via Phabricator via cfe-commits]

vglavnyy created this revision.
vglavnyy added reviewers: djasper, krasimir.
Herald added a subscriber: cfe-commits.

Twice running clang-format may give unstable result for some code samples, for 
example: https://bugs.llvm.org/show_bug.cgi?id=23728
This commit adds stability check to clang-format unit-tests.
After apply this patch 10 tests from the FormatTests will fail.

> [==] 647 tests from 19 test cases ran. (31449 ms total)
> [  PASSED  ] 637 tests.
> [  FAILED  ] 10 tests, listed below:
> [  FAILED  ] FormatTest.OnlyGeneratesNecessaryReplacements
> [  FAILED  ] FormatTest.BreaksStringLiterals
> [  FAILED  ] FormatTest.BreaksStringLiteralsWithTabs
> [  FAILED  ] FormatTest.BreaksWideAndNSStringLiterals
> [  FAILED  ] FormatTest.BreaksStringLiteralsWithin_TMacro
> [  FAILED  ] FormatTest.DoNotBreakStringLiteralsInEscapeSequence
> [  FAILED  ] FormatTest.OptimizeBreakPenaltyVsExcess
> [  FAILED  ] FormatTest.WorksFor8bitEncodings
> [  FAILED  ] FormatTest.SplitsUTF8Strings
> [  FAILED  ] FormatTest.SupportsCRLF


Repository:
  rC Clang

https://reviews.llvm.org/D53482

Files:
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -38,7 +38,8 @@
 
   std::string format(llvm::StringRef Code,
  const FormatStyle  = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+ StatusCheck CheckComplete = SC_ExpectComplete,
+ bool check = true) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
 std::vector Ranges(1, tooling::Range(0, Code.size()));
@@ -54,6 +55,12 @@
 auto Result = applyAllReplacements(Code, Replaces);
 EXPECT_TRUE(static_cast(Result));
 LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+if (check && Status.FormatComplete) {
+  // Do recursive call and check that format of format is valid.
+  LLVM_DEBUG(llvm::errs() << "\ncheck\n\n");
+  auto FormatedResult = format(*Result, Style, SC_ExpectComplete, false);
+  EXPECT_EQ(*Result, FormatedResult);
+}
 return *Result;
   }
 


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -38,7 +38,8 @@
 
   std::string format(llvm::StringRef Code,
  const FormatStyle  = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+ StatusCheck CheckComplete = SC_ExpectComplete,
+ bool check = true) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
 std::vector Ranges(1, tooling::Range(0, Code.size()));
@@ -54,6 +55,12 @@
 auto Result = applyAllReplacements(Code, Replaces);
 EXPECT_TRUE(static_cast(Result));
 LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+if (check && Status.FormatComplete) {
+  // Do recursive call and check that format of format is valid.
+  LLVM_DEBUG(llvm::errs() << "\ncheck\n\n");
+  auto FormatedResult = format(*Result, Style, SC_ExpectComplete, false);
+  EXPECT_EQ(*Result, FormatedResult);
+}
 return *Result;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits