[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 326957.
poelmanc added a comment.

Removed a function in modernize/LoopConvertCheck.cpp that's no longer needed 
due to this patch. Fixed clang-format and clang-tidy issues identified by 
HarborMaster in prior submission.

Hopefully it's ready to go, but as always I'm happy to receive any feedback.


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

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/unittests/clang-change-namespace/ChangeNamespaceTests.cpp
  clang/include/clang/Basic/CharInfo.h
  clang/include/clang/Format/Format.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/CleanupTest.cpp

Index: clang/unittests/Format/CleanupTest.cpp
===
--- clang/unittests/Format/CleanupTest.cpp
+++ clang/unittests/Format/CleanupTest.cpp
@@ -11,6 +11,7 @@
 #include "../Tooling/ReplacementTest.h"
 #include "clang/Tooling/Core/Replacement.h"
 
+#include "llvm/Testing/Support/Annotations.h"
 #include "gtest/gtest.h"
 
 using clang::tooling::ReplacementTest;
@@ -320,6 +321,11 @@
 return tooling::Replacement(FileName, Offset, Length, Text);
   }
 
+  tooling::Replacement createReplacement(llvm::Annotations::Range Range,
+ StringRef Text) {
+return createReplacement(Range.Begin, Range.End - Range.Begin, Text);
+  }
+
   tooling::Replacement createInsertion(StringRef IncludeDirective) {
 return createReplacement(UINT_MAX, 0, IncludeDirective);
   }
@@ -373,10 +379,12 @@
  "namespace C {\n"
  "namespace D { int i; }\n"
  "inline namespace E { namespace { int y; } }\n"
+ "\n"
  "int x= 0;"
  "}";
-  std::string Expected = "\n\nnamespace C {\n"
- "namespace D { int i; }\n\n"
+  std::string Expected = "\nnamespace C {\n"
+ "namespace D { int i; }\n"
+ "\n"
  "int x= 0;"
  "}";
   tooling::Replacements Replaces =
@@ -386,6 +394,104 @@
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code("namespace A {$r1[[ // Useless comment]]\n"
+ "  $r2[[int]] $r3[[x]]\t $r4[[=]] $r5[[0;]]\t\n"
+ "  int y\t = 0;$r6[[\t]]\n"
+ "} // namespace A\n");
+  std::string Expected = "namespace A {\n"
+ "  int y\t = 0;\n"
+ "} // namespace A\n";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), ""),
+  createReplacement(Code.range("r5"), ""),
+  createReplacement(Code.range("r6"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, RemoveLinesWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code(R"cpp(struct A {
+  A()
+  $r3[[:]] $r1[[f()]]$r2[[,]]
+$r4[[g()]]
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp");
+  std::string Expected = R"cpp(struct A {
+  A()
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, KeepLinesWithInsertsOrReplacesEvenIfBlank) {
+  // Not using raw string literals so newlines and spaces are clear and explicit
+  llvm::Annotations Code("struct A {\n"
+ "  A() {}\n"
+ "$r3[[]]  $r1[[int]] $r2[[f;]]\n" // "  int f;\n"
+ "  \n"
+ "$r4[[  ]]\n"
+ "};");
+  std::string Expected = "struct A {\n"
+ "  A() 

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D68682#2547906 , @kadircet wrote:

> It looks like premerge tests skipped your last diff with id 321451 
> (https://reviews.llvm.org/harbormaster/?after=87950). You can re-trigger by 
> uploading a new diff, in the meantime i would also file a bug to 
> https://github.com/google/llvm-premerge-checks/issues. mentioning your diff 
> id.

Thanks @kadircet, actually just reviewing the current bug list explained the 
problem: https://github.com/google/llvm-premerge-checks/issues/263, //Build is 
not triggered if diff repository is not set//. I wasn't selecting //rG 
 LLVM Github Monorepo// per 
instructions at https://llvm.org/docs/Phabricator.html that said //Leave the 
Repository field blank//.

HarborMaster has now run and passes all tests on Windows and Linux.

1. clang-format for a new test case in 
`clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp`.
 That test case formatting matches real-world examples so I'm reluctant to 
change it - I believe it's clang-formatted but with different settings from 
llvm-project's.
2. clang-tidy finds 8 `int` vs `size_t` comparisons. While `int` feels safer 
when subtraction is involved (since one can `assert` variables stay positive), 
this particular code would work fine with `int` variables changed to `size_t`, 
or alternatively with `static_cast` to silence the warnings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68682

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

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 322265.
poelmanc added a comment.

Call `llvm::Annoation` constructor rather than `operator=` to fix linux build 
issue, fix some issues identified by clang-format and clang-tidy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/unittests/clang-change-namespace/ChangeNamespaceTests.cpp
  clang/include/clang/Basic/CharInfo.h
  clang/include/clang/Format/Format.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/CleanupTest.cpp

Index: clang/unittests/Format/CleanupTest.cpp
===
--- clang/unittests/Format/CleanupTest.cpp
+++ clang/unittests/Format/CleanupTest.cpp
@@ -11,6 +11,7 @@
 #include "../Tooling/ReplacementTest.h"
 #include "clang/Tooling/Core/Replacement.h"
 
+#include "llvm/Testing/Support/Annotations.h"
 #include "gtest/gtest.h"
 
 using clang::tooling::ReplacementTest;
@@ -320,6 +321,11 @@
 return tooling::Replacement(FileName, Offset, Length, Text);
   }
 
+  tooling::Replacement createReplacement(llvm::Annotations::Range Range,
+ StringRef Text) {
+return createReplacement(Range.Begin, Range.End - Range.Begin, Text);
+  }
+
   tooling::Replacement createInsertion(StringRef IncludeDirective) {
 return createReplacement(UINT_MAX, 0, IncludeDirective);
   }
@@ -373,10 +379,12 @@
  "namespace C {\n"
  "namespace D { int i; }\n"
  "inline namespace E { namespace { int y; } }\n"
+ "\n"
  "int x= 0;"
  "}";
-  std::string Expected = "\n\nnamespace C {\n"
- "namespace D { int i; }\n\n"
+  std::string Expected = "\nnamespace C {\n"
+ "namespace D { int i; }\n"
+ "\n"
  "int x= 0;"
  "}";
   tooling::Replacements Replaces =
@@ -386,6 +394,104 @@
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code("namespace A {$r1[[ // Useless comment]]\n"
+ "  $r2[[int]] $r3[[x]]\t $r4[[=]] $r5[[0;]]\t\n"
+ "  int y\t = 0;$r6[[\t]]\n"
+ "} // namespace A\n");
+  std::string Expected = "namespace A {\n"
+ "  int y\t = 0;\n"
+ "} // namespace A\n";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), ""),
+  createReplacement(Code.range("r5"), ""),
+  createReplacement(Code.range("r6"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, RemoveLinesWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code(R"cpp(struct A {
+  A()
+  $r3[[:]] $r1[[f()]]$r2[[,]]
+$r4[[g()]]
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp");
+  std::string Expected = R"cpp(struct A {
+  A()
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, KeepLinesWithInsertsOrReplacesEvenIfBlank) {
+  // Not using raw string literals so newlines and spaces are clear and explicit
+  llvm::Annotations Code("struct A {\n"
+ "  A() {}\n"
+ "$r3[[]]  $r1[[int]] $r2[[f;]]\n" // "  int f;\n"
+ "  \n"
+ "$r4[[  ]]\n"
+ "};");
+  std::string Expected = "struct A {\n"
+ "  A() {}\n"
+ "\n"
+ "\n"
+ "  \n"
+ "\t\n"
+  

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 322108.
poelmanc added a comment.

Comment change, "beginning" to "start" for consistency, being sure to set 
Repository on "diff" page (not just on edit page) to see if 
https://github.com/google/llvm-premerge-checks/issues/263 was the problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/unittests/clang-change-namespace/ChangeNamespaceTests.cpp
  clang/include/clang/Basic/CharInfo.h
  clang/include/clang/Format/Format.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/CleanupTest.cpp

Index: clang/unittests/Format/CleanupTest.cpp
===
--- clang/unittests/Format/CleanupTest.cpp
+++ clang/unittests/Format/CleanupTest.cpp
@@ -12,6 +12,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 
 #include "gtest/gtest.h"
+#include "llvm/Testing/Support/Annotations.h"
 
 using clang::tooling::ReplacementTest;
 using clang::tooling::toReplacements;
@@ -320,6 +321,11 @@
 return tooling::Replacement(FileName, Offset, Length, Text);
   }
 
+  tooling::Replacement createReplacement(llvm::Annotations::Range Range,
+ StringRef Text) {
+return createReplacement(Range.Begin, Range.End - Range.Begin, Text);
+  }
+
   tooling::Replacement createInsertion(StringRef IncludeDirective) {
 return createReplacement(UINT_MAX, 0, IncludeDirective);
   }
@@ -373,10 +379,12 @@
  "namespace C {\n"
  "namespace D { int i; }\n"
  "inline namespace E { namespace { int y; } }\n"
+ "\n"
  "int x= 0;"
  "}";
-  std::string Expected = "\n\nnamespace C {\n"
- "namespace D { int i; }\n\n"
+  std::string Expected = "\nnamespace C {\n"
+ "namespace D { int i; }\n"
+ "\n"
  "int x= 0;"
  "}";
   tooling::Replacements Replaces =
@@ -386,6 +394,104 @@
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code = "namespace A {$r1[[ // Useless comment]]\n"
+   "  $r2[[int]] $r3[[x]]\t $r4[[=]] $r5[[0;]]\t\n"
+   "  int y\t = 0;$r6[[\t]]\n"
+   "} // namespace A\n";
+  std::string Expected = "namespace A {\n"
+ "  int y\t = 0;\n"
+ "} // namespace A\n";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), ""),
+  createReplacement(Code.range("r5"), ""),
+  createReplacement(Code.range("r6"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, RemoveLinesWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code = R"cpp(struct A {
+  A()
+  $r3[[:]] $r1[[f()]]$r2[[,]]
+$r4[[g()]]
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  std::string Expected = R"cpp(struct A {
+  A()
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, KeepLinesWithInsertsOrReplacesEvenIfBlank) {
+  // Not using raw string literals so newlines and spaces are clear and explicit
+  llvm::Annotations Code = "struct A {\n"
+ "  A() {}\n"
+ "$r3[[]]  $r1[[int]] $r2[[f;]]\n" // "  int f;\n"
+ "  \n"
+ "$r4[[  ]]\n"
+ "};";
+  std::string Expected = "struct A {\n"
+ "  A() {}\n"
+ "\n"
+ "\n"
+   

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 322033.
poelmanc added a comment.

Update one one comment, hopefully trigger HarborMaster to rerun.


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

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/unittests/clang-change-namespace/ChangeNamespaceTests.cpp
  clang/include/clang/Basic/CharInfo.h
  clang/include/clang/Format/Format.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/CleanupTest.cpp

Index: clang/unittests/Format/CleanupTest.cpp
===
--- clang/unittests/Format/CleanupTest.cpp
+++ clang/unittests/Format/CleanupTest.cpp
@@ -12,6 +12,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 
 #include "gtest/gtest.h"
+#include "llvm/Testing/Support/Annotations.h"
 
 using clang::tooling::ReplacementTest;
 using clang::tooling::toReplacements;
@@ -320,6 +321,11 @@
 return tooling::Replacement(FileName, Offset, Length, Text);
   }
 
+  tooling::Replacement createReplacement(llvm::Annotations::Range Range,
+ StringRef Text) {
+return createReplacement(Range.Begin, Range.End - Range.Begin, Text);
+  }
+
   tooling::Replacement createInsertion(StringRef IncludeDirective) {
 return createReplacement(UINT_MAX, 0, IncludeDirective);
   }
@@ -373,10 +379,12 @@
  "namespace C {\n"
  "namespace D { int i; }\n"
  "inline namespace E { namespace { int y; } }\n"
+ "\n"
  "int x= 0;"
  "}";
-  std::string Expected = "\n\nnamespace C {\n"
- "namespace D { int i; }\n\n"
+  std::string Expected = "\nnamespace C {\n"
+ "namespace D { int i; }\n"
+ "\n"
  "int x= 0;"
  "}";
   tooling::Replacements Replaces =
@@ -386,6 +394,104 @@
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code = "namespace A {$r1[[ // Useless comment]]\n"
+   "  $r2[[int]] $r3[[x]]\t $r4[[=]] $r5[[0;]]\t\n"
+   "  int y\t = 0;$r6[[\t]]\n"
+   "} // namespace A\n";
+  std::string Expected = "namespace A {\n"
+ "  int y\t = 0;\n"
+ "} // namespace A\n";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), ""),
+  createReplacement(Code.range("r5"), ""),
+  createReplacement(Code.range("r6"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, RemoveLinesWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code = R"cpp(struct A {
+  A()
+  $r3[[:]] $r1[[f()]]$r2[[,]]
+$r4[[g()]]
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  std::string Expected = R"cpp(struct A {
+  A()
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, KeepLinesWithInsertsOrReplacesEvenIfBlank) {
+  // Not using raw string literals so newlines and spaces are clear and explicit
+  llvm::Annotations Code = "struct A {\n"
+ "  A() {}\n"
+ "$r3[[]]  $r1[[int]] $r2[[f;]]\n" // "  int f;\n"
+ "  \n"
+ "$r4[[  ]]\n"
+ "};";
+  std::string Expected = "struct A {\n"
+ "  A() {}\n"
+ "\n"
+ "\n"
+ "  \n"
+ "\t\n"
+ "};";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), "   "),

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

It looks like premerge tests skipped your last diff with id 321451 
(https://reviews.llvm.org/harbormaster/?after=87950). You can re-trigger by 
uploading a new diff, in the meantime i would also file a bug to 
https://github.com/google/llvm-premerge-checks/issues. mentioning your diff id.


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

https://reviews.llvm.org/D68682

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

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

On 2 Feb Harbormaster found a bug  from my 
switching some char* code to use StringRef. I uploaded a new patch on 4 Feb, 
but Harbormaster does not appear to have rerun. What triggers Harbormaster - do 
I need to take some action? I haven't been able to find such options myself.

Please holler if there's a better place to ask this, thanks!


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

https://reviews.llvm.org/D68682

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

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 321451.
poelmanc added a comment.
Herald added a subscriber: nullptr.cpp.

Change loop end condition in `findLineEnd` and add several `assert` statements.


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

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/unittests/clang-change-namespace/ChangeNamespaceTests.cpp
  clang/include/clang/Basic/CharInfo.h
  clang/include/clang/Format/Format.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/CleanupTest.cpp

Index: clang/unittests/Format/CleanupTest.cpp
===
--- clang/unittests/Format/CleanupTest.cpp
+++ clang/unittests/Format/CleanupTest.cpp
@@ -12,6 +12,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 
 #include "gtest/gtest.h"
+#include "llvm/Testing/Support/Annotations.h"
 
 using clang::tooling::ReplacementTest;
 using clang::tooling::toReplacements;
@@ -320,6 +321,11 @@
 return tooling::Replacement(FileName, Offset, Length, Text);
   }
 
+  tooling::Replacement createReplacement(llvm::Annotations::Range Range,
+ StringRef Text) {
+return createReplacement(Range.Begin, Range.End - Range.Begin, Text);
+  }
+
   tooling::Replacement createInsertion(StringRef IncludeDirective) {
 return createReplacement(UINT_MAX, 0, IncludeDirective);
   }
@@ -373,10 +379,12 @@
  "namespace C {\n"
  "namespace D { int i; }\n"
  "inline namespace E { namespace { int y; } }\n"
+ "\n"
  "int x= 0;"
  "}";
-  std::string Expected = "\n\nnamespace C {\n"
- "namespace D { int i; }\n\n"
+  std::string Expected = "\nnamespace C {\n"
+ "namespace D { int i; }\n"
+ "\n"
  "int x= 0;"
  "}";
   tooling::Replacements Replaces =
@@ -386,6 +394,104 @@
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code = "namespace A {$r1[[ // Useless comment]]\n"
+   "  $r2[[int]] $r3[[x]]\t $r4[[=]] $r5[[0;]]\t\n"
+   "  int y\t = 0;$r6[[\t]]\n"
+   "} // namespace A\n";
+  std::string Expected = "namespace A {\n"
+ "  int y\t = 0;\n"
+ "} // namespace A\n";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), ""),
+  createReplacement(Code.range("r5"), ""),
+  createReplacement(Code.range("r6"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, RemoveLinesWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code = R"cpp(struct A {
+  A()
+  $r3[[:]] $r1[[f()]]$r2[[,]]
+$r4[[g()]]
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  std::string Expected = R"cpp(struct A {
+  A()
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, KeepLinesWithInsertsOrReplacesEvenIfBlank) {
+  // Not using raw string literals so newlines and spaces are clear and explicit
+  llvm::Annotations Code = "struct A {\n"
+ "  A() {}\n"
+ "$r3[[]]  $r1[[int]] $r2[[f;]]\n" // "  int f;\n"
+ "  \n"
+ "$r4[[  ]]\n"
+ "};";
+  std::string Expected = "struct A {\n"
+ "  A() {}\n"
+ "\n"
+ "\n"
+ "  \n"
+ "\t\n"
+ "};";
+  tooling::Replacements Replaces =
+  

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked 14 inline comments as done.
poelmanc added inline comments.



Comment at: clang/lib/Format/Format.cpp:2381
+// if so adds a Replacement to NewReplacements that removes the entire line.
+llvm::Error MaybeRemoveBlankLine(tooling::Replacements ,
+ StringRef FilePath, StringRef Code,

kadircet wrote:
> kadircet wrote:
> > can we rather make this function return a `tooling::Replacement` ?
> name should be `maybeRemoveBlankLine`
`maybeRemoveBlankLine` needs to be called twice so I tried to put maximum logic 
in it so to avoid replicating logic at the two calling sites. It doesn't always 
create a replacement, so I believe we could return 
`optional`. Then the caller would have to check the 
`optional`, and if set  call `NewReplaces.add()`, then check that result... I 
can do that if you like but please take a fresh look at the updated 
implementation. Thanks!



Comment at: clang/lib/Format/Format.cpp:2394
+assert(LineEndPos >= CheckedThroughPos);
+StringRef TrailingText(FileText + CheckedThroughPos,
+   LineEndPos - CheckedThroughPos);

kadircet wrote:
> ```
> if(!isAllWhiteSpace(Code.substr(CheckedThroughPos, LineEndPos - 
> CheckedThroughPos))
>   return llvm::Error::success();
> ```
Coming back after a year I found my own code here a bit hard to read... Your 
feedback prompted me to use `StringRef` over `char*` and reduce the number of 
intermediate variables. I put your two suggested `isAllWhiteSpace` calls 
`isAllWhiteSpace` together with `&&`, feel free to suggest further improvement.




Comment at: clang/lib/Format/Format.cpp:2397
+assert(LineEndPos >= LineStartPos);
+StringRef OriginalLine(FileText + LineStartPos, LineEndPos - LineStartPos);
+if (isAllWhitespace(TrailingText) && !isAllWhitespace(OriginalLine)) {

kadircet wrote:
> ```
> if(isAllWhiteSpace(Code.substr(LineStartPos, CheckedThroughPos - 
> LineStartPos))
>   return llvm::Error::success();
> ```
> 
> Also move this to the top, as it doesn't actually require any of the 
> computations you performed.
> 
> I would actually keep a `HasDeleted` flag in the caller, updating it by 
> looking at length of replacements, and call this function iff we've deleted 
> something in that line, but up to you.
Used suggested `isAllWhitespace` together in the `if` clause with above 
`isAllWhitespace`.



Comment at: clang/lib/Format/Format.cpp:2403
+  (CheckedThroughPos > 0 &&
+   isVerticalWhitespace(FileText[CheckedThroughPos - 1]))
+  ? (FileText + LineEndPos)

kadircet wrote:
> i don't follow what this check is about.
> 
> the comment talks about a replacement removing a trailing newline, but check 
> is for a preceding whitespace, and it is not even at the `LineEndPos` ?
> how come we get a vertical whitespace, in the middle of a line?
I clarified the comment to include an example: when the code is `...  
foo\n\n...` and `foo\n` is removed by a replacement already, the second `\n` 
actually represents the //next// line which we don't want to remove.



Comment at: clang/unittests/Format/CleanupTest.cpp:368
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  std::string Code = "namespace A { // Useless comment\n"
+ "  int x\t = 0;\t\n"

kadircet wrote:
> poelmanc wrote:
> > kadircet wrote:
> > > could you replace these with raw string literals instead to get rid of 
> > > `\n`s.
> > I like that modernization suggestion but as a fairly new llvm contributor I 
> > follow the style I see in the surrounding code. Perhaps we should submit a 
> > seperate patch to modernize whole files at a time to use raw string 
> > literals and see how the change is received by the community.
> modernization of the existing code, and introducing old code are different 
> things. former is harder because we would like to keep the history clean, but 
> we tend to do the latter to make sure quality improves over time.
> 
> feel free to keep it if you want though, this one isn't so important.
I tried raw string literals, but these particular tests are all about tabs, 
spaces, and newlines which I found harder to see and reason about using raw 
string literals. I kept the raw string literals in 
`RemoveLinesWhenAllNonWhitespaceRemoved`.



Comment at: clang/unittests/Format/CleanupTest.cpp:376
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 1, 14), 19, ""),
+  createReplacement(getOffset(Code, 2, 3), 3, ""),

kadircet wrote:
> poelmanc wrote:
> > kadircet wrote:
> > > can you make use of `Annotations` library in 
> > > `llvm/include/llvm/Testing/Support/Annotations.h` for getting offsets 
> > > into the code?
> > > 
> > > same for the following test cases
> > 

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 320678.
poelmanc changed the repository for this revision from rCTE Clang Tools Extra 
to rG LLVM Github Monorepo.
poelmanc added a comment.
Herald added subscribers: dexonsmith, mgorny.

Glad to be back after a year away from clang-tidy, and sorry to have let this 
patch linger. Running clang-tidy over a large codebase shows this patch still 
to be needed. I believe I've addressed all identified issues but welcome 
feedback.

@gribozavr @gribozavr2 requested changes to a very early version of this patch 
and I later reworked it to follow the `cleanupAroundReplacements` approach he 
suggested on 2019-10-11.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/unittests/clang-change-namespace/ChangeNamespaceTests.cpp
  clang/include/clang/Basic/CharInfo.h
  clang/include/clang/Format/Format.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/CleanupTest.cpp

Index: clang/unittests/Format/CleanupTest.cpp
===
--- clang/unittests/Format/CleanupTest.cpp
+++ clang/unittests/Format/CleanupTest.cpp
@@ -12,6 +12,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 
 #include "gtest/gtest.h"
+#include "llvm/Testing/Support/Annotations.h"
 
 using clang::tooling::ReplacementTest;
 using clang::tooling::toReplacements;
@@ -320,6 +321,11 @@
 return tooling::Replacement(FileName, Offset, Length, Text);
   }
 
+  tooling::Replacement createReplacement(llvm::Annotations::Range Range,
+ StringRef Text) {
+return createReplacement(Range.Begin, Range.End - Range.Begin, Text);
+  }
+
   tooling::Replacement createInsertion(StringRef IncludeDirective) {
 return createReplacement(UINT_MAX, 0, IncludeDirective);
   }
@@ -373,10 +379,12 @@
  "namespace C {\n"
  "namespace D { int i; }\n"
  "inline namespace E { namespace { int y; } }\n"
+ "\n"
  "int x= 0;"
  "}";
-  std::string Expected = "\n\nnamespace C {\n"
- "namespace D { int i; }\n\n"
+  std::string Expected = "\nnamespace C {\n"
+ "namespace D { int i; }\n"
+ "\n"
  "int x= 0;"
  "}";
   tooling::Replacements Replaces =
@@ -386,6 +394,104 @@
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code = "namespace A {$r1[[ // Useless comment]]\n"
+   "  $r2[[int]] $r3[[x]]\t $r4[[=]] $r5[[0;]]\t\n"
+   "  int y\t = 0;$r6[[\t]]\n"
+   "} // namespace A\n";
+  std::string Expected = "namespace A {\n"
+ "  int y\t = 0;\n"
+ "} // namespace A\n";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), ""),
+  createReplacement(Code.range("r5"), ""),
+  createReplacement(Code.range("r6"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, RemoveLinesWhenAllNonWhitespaceRemoved) {
+  llvm::Annotations Code = R"cpp(struct A {
+  A()
+  $r3[[:]] $r1[[f()]]$r2[[,]]
+$r4[[g()]]
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  std::string Expected = R"cpp(struct A {
+  A()
+  {}
+  int f = 0;
+  int g = 0;
+};)cpp";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(Code.range("r1"), ""),
+  createReplacement(Code.range("r2"), ""),
+  createReplacement(Code.range("r3"), ""),
+  createReplacement(Code.range("r4"), "")});
+
+  EXPECT_EQ(Expected, apply(Code.code(), Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, KeepLinesWithInsertsOrReplacesEvenIfBlank) {
+  // Not using raw string literals so newlines and spaces are clear and explicit
+  llvm::Annotations Code = "struct A 

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 2 inline comments as done.
kadircet added inline comments.



Comment at: clang/lib/Format/Format.cpp:2381
+// if so adds a Replacement to NewReplacements that removes the entire line.
+llvm::Error MaybeRemoveBlankLine(tooling::Replacements ,
+ StringRef FilePath, StringRef Code,

can we rather make this function return a `tooling::Replacement` ?



Comment at: clang/lib/Format/Format.cpp:2381
+// if so adds a Replacement to NewReplacements that removes the entire line.
+llvm::Error MaybeRemoveBlankLine(tooling::Replacements ,
+ StringRef FilePath, StringRef Code,

kadircet wrote:
> can we rather make this function return a `tooling::Replacement` ?
name should be `maybeRemoveBlankLine`



Comment at: clang/lib/Format/Format.cpp:2383
+ StringRef FilePath, StringRef Code,
+ bool LineBlankSoFar, int LineStartPos,
+ int CheckedThroughPos) {

instead of passing `LineBlankSoFar` as a parameter, maybe don't call the 
function at all if it doesn't hold?



Comment at: clang/lib/Format/Format.cpp:2394
+assert(LineEndPos >= CheckedThroughPos);
+StringRef TrailingText(FileText + CheckedThroughPos,
+   LineEndPos - CheckedThroughPos);

```
if(!isAllWhiteSpace(Code.substr(CheckedThroughPos, LineEndPos - 
CheckedThroughPos))
  return llvm::Error::success();
```



Comment at: clang/lib/Format/Format.cpp:2397
+assert(LineEndPos >= LineStartPos);
+StringRef OriginalLine(FileText + LineStartPos, LineEndPos - LineStartPos);
+if (isAllWhitespace(TrailingText) && !isAllWhitespace(OriginalLine)) {

```
if(isAllWhiteSpace(Code.substr(LineStartPos, CheckedThroughPos - LineStartPos))
  return llvm::Error::success();
```

Also move this to the top, as it doesn't actually require any of the 
computations you performed.

I would actually keep a `HasDeleted` flag in the caller, updating it by looking 
at length of replacements, and call this function iff we've deleted something 
in that line, but up to you.



Comment at: clang/lib/Format/Format.cpp:2403
+  (CheckedThroughPos > 0 &&
+   isVerticalWhitespace(FileText[CheckedThroughPos - 1]))
+  ? (FileText + LineEndPos)

i don't follow what this check is about.

the comment talks about a replacement removing a trailing newline, but check is 
for a preceding whitespace, and it is not even at the `LineEndPos` ?
how come we get a vertical whitespace, in the middle of a line?



Comment at: clang/lib/Format/Format.cpp:2416
+// Find the beginning of the line containing StartPos.
+int FindLineStart(const char *FileText, int StartPos) {
+  int LineStartPos = StartPos;

name should be `findLineStart`



Comment at: clang/lib/Format/Format.cpp:2417
+int FindLineStart(const char *FileText, int StartPos) {
+  int LineStartPos = StartPos;
+  // When preceding character is '\n' or '\r', LineStartPos is start of line.

nit: no need to copy the parameter(i.e. creating a new variable)



Comment at: clang/lib/Format/Format.cpp:2453
+  MaybeRemoveBlankLine(NewReplaces, FilePath, Code, LineBlankSoFar,
+   LineStartPos, LineCheckedThroughPos))
+return std::move(Err);

both `LineStartPos` and `LineCheckedThroughPos` would be `-1` for the first 
replacement in the file. and you will use them to index into `Code`.

I believe it makes sense for those to be initialized to `0`.



Comment at: clang/lib/Format/Format.cpp:2466
+LineBlankSoFar = LineBlankSoFar && isAllWhitespace(PriorTextToCheck) &&
+ ReplacementText.empty();
+LineCheckedThroughPos = R.getOffset() + R.getLength();

nit: inline the variable, i.e `R.getReplacementText().empty()`



Comment at: clang/lib/Format/Format.cpp:2467
+ ReplacementText.empty();
+LineCheckedThroughPos = R.getOffset() + R.getLength();
+  }

nit: `RStartPos + R.getLength();`



Comment at: clang/lib/Format/Format.cpp:2391
+
+int CurrentRLineStartPos = RStartPos;
+while (CurrentRLineStartPos > 0 &&

poelmanc wrote:
> kadircet wrote:
> > it seems like this loop is trying to find line start, would be nice to 
> > clarify with a comment.
> > 
> > If so seems like the logic is a little buggy, for example:
> > 
> > `int a^;` if the offset is at `^`(namely at `a`) then this will return 
> > current position as line start, since `a` is preceded by a whitespace.
> > Maybe change the logic to find the first `\n` before current offset, and 
> > skip any 

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked 4 inline comments as done.
poelmanc added a comment.

In D68682#1754798 , @kadircet wrote:

> `ToolingTests/ApplyAtomicChangesTest.FormatsCorrectLineWhenHeaderIsRemoved` 
> also seems to be failing, you can run it with `ninja ToolingTests && 
> ./tools/clang/unittests/Tooling/ToolingTests 
> --gtest_filter="ApplyAtomicChangesTest.FormatsCorrectLineWhenHeaderIsRemoved"`


Thank you, this test revealed a real edge case for which the algorithm was 
failing, which I've now fixed and created a new test case to verify: If a set 
of Replacements removes all non-whitespace on a line **including** the trailing 
newline **and** the line is followed by a single newline character, it should 
not chomp the following newline as well.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682



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

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked an inline comment as done.
poelmanc added inline comments.



Comment at: clang/include/clang/Basic/CharInfo.h:94
+LLVM_READONLY inline bool isWhitespace(llvm::StringRef S) {
+  for (StringRef::const_iterator I = S.begin(), E = S.end(); I != E; ++I) {
+if (!isWhitespace(*I))

alexfh wrote:
> I'd suggest to avoid overloading here. A name like `isWhitespaceOnly` would 
> be less ambiguous.
> 
> As for implementation, it can be less verbose:
> ```
> for (char C : S)
>   if (!isWhitespace(*I))
> return false;
> return true;
> ```
> or just
> ```
> return llvm::all_of(S, isWhitespace);
> ```
Thanks for the suggestion, I've removed the overloading and updated the 
implementation.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682



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

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked 13 inline comments as done.
poelmanc added inline comments.



Comment at: clang/lib/Format/Format.cpp:2385
+  const char *FileText = Code.data();
+  StringRef FilePath; // Must be the same for every Replacement
+  for (const auto  : Replaces) {

kadircet wrote:
> maybe assign `FilePath = Replaces.begin()->getFilePath()` here, and early 
> exit in the beginning if `Replaces` is empty?
> Then you can just `assert(FilePath == R.getFilePath())`
Thank you for the detailed review of the algorithm and the suggestions, sorry 
it took me so long to get back to this. Done.



Comment at: clang/lib/Format/Format.cpp:2391
+
+int CurrentRLineStartPos = RStartPos;
+while (CurrentRLineStartPos > 0 &&

kadircet wrote:
> it seems like this loop is trying to find line start, would be nice to 
> clarify with a comment.
> 
> If so seems like the logic is a little buggy, for example:
> 
> `int a^;` if the offset is at `^`(namely at `a`) then this will return 
> current position as line start, since `a` is preceded by a whitespace.
> Maybe change the logic to find the first `\n` before current offset, and skip 
> any whitespaces after that `\n` (if really necessary)
> 
> btw, please do explain why you are skipping the preceding whitespaces in the 
> comment.
> 
> could you also add a test case for this one?
Good point, I clarified by refactoring this code into a simple helper function 
`FindLineStart`.

I don't believe there's buggy logic but the term //vertical whitespace// may 
have caused confusion - it includes only newline characters (linefeed & 
carriage returns) whereas //whitespace// includes those plus spaces and tabs. 
Now that the function is called `FindLineStart` the logic is hopefully clear.



Comment at: clang/lib/Format/Format.cpp:2398
+assert(CurrentRLineStartPos >= LineStartPos);
+if (CurrentRLineStartPos != LineStartPos) {
+  // We've moved on to a new line. Wrap up the old one before moving on.

kadircet wrote:
> this logic might not work in cases like:
> 
> ```
> in^t a;
> int^ b;
> ```
> 
> let's assume you've got replacements at places marked with `^` again, then 
> they would have same start position so you would miss the line change event.
> maybe count number of `\n`s in the previous loop to detect current line 
> number.
> 
> Also please add a test for this case as well.
I'm not sure I understand this concern; if you could draft up a specific test 
case I'd be happy to ensure it works.



Comment at: clang/lib/Format/Format.cpp:2427
+  // (b) the original line was *not* blank.
+  for (const auto  : PotentialWholeLineCuts) {
+const int LineStartPos = LineCheckedThrough.first;

kadircet wrote:
> the second loop seems like an over-kill and complicates the code a lot.
> 
> IIUC, it is just checking whether any text after replacements is 
> "whitespace/newline" and if so deletes the whole line.
> Instead of doing all of this, in the previous loop, whenever we see a line 
> change could we scan until the EOL, and delete the EOL token if we didn't hit 
> anything but whitespaces?
> 
> this would mean you won't need to store `PotentialWholeLineCuts`.
I agree and thanks for the feedback! `PotentialWholeLineCuts` felt a bit hacky 
so I've eliminated it by creating a new `MaybeRemoveBlankLine` helper function 
that gets called in two places, eliminating the need for the second pass and 
the `PotentialWholeLineCuts` variable. Hopefully the code is easier to follow 
now.



Comment at: clang/unittests/Format/CleanupTest.cpp:368
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  std::string Code = "namespace A { // Useless comment\n"
+ "  int x\t = 0;\t\n"

kadircet wrote:
> could you replace these with raw string literals instead to get rid of `\n`s.
I like that modernization suggestion but as a fairly new llvm contributor I 
follow the style I see in the surrounding code. Perhaps we should submit a 
seperate patch to modernize whole files at a time to use raw string literals 
and see how the change is received by the community.



Comment at: clang/unittests/Format/CleanupTest.cpp:376
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 1, 14), 19, ""),
+  createReplacement(getOffset(Code, 2, 3), 3, ""),

kadircet wrote:
> can you make use of `Annotations` library in 
> `llvm/include/llvm/Testing/Support/Annotations.h` for getting offsets into 
> the code?
> 
> same for the following test cases
`Annotations` looks quite handy, but again I'd rather get this patch through 
consistent with the file's existing style and submit a separate patch to 
modernize a whole file at a time to use `Annotations`.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 235090.
poelmanc marked 2 inline comments as done.
poelmanc added a comment.

Fix algorithm to fix failing 
`ToolingTests/ApplyAtomicChangesTest.FormatsCorrectLineWhenHeaderIsRemoved`. 
That test revealed a very specific but real failure case: if existing Removals 
removed all whitespace **including** the line's ending newline, **and** the 
subsequent line contained only a newline, it would delete the subsequent 
newline too, which was not desired. Added a new test 
`CleanUpReplacementsTest.RemoveLineAndNewlineLineButNotNext` to explicitly test 
this case.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/unittests/clang-change-namespace/ChangeNamespaceTests.cpp
  clang/include/clang/Basic/CharInfo.h
  clang/include/clang/Format/Format.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/CleanupTest.cpp

Index: clang/unittests/Format/CleanupTest.cpp
===
--- clang/unittests/Format/CleanupTest.cpp
+++ clang/unittests/Format/CleanupTest.cpp
@@ -349,10 +349,12 @@
  "namespace C {\n"
  "namespace D { int i; }\n"
  "inline namespace E { namespace { int y; } }\n"
+ "\n"
  "int x= 0;"
  "}";
-  std::string Expected = "\n\nnamespace C {\n"
- "namespace D { int i; }\n\n"
+  std::string Expected = "\nnamespace C {\n"
+ "namespace D { int i; }\n"
+ "\n"
  "int x= 0;"
  "}";
   tooling::Replacements Replaces =
@@ -362,6 +364,100 @@
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  std::string Code = "namespace A { // Useless comment\n"
+ "  int x\t = 0;\t\n"
+ "  int y\t = 0;\t\n"
+ "} // namespace A\n";
+  std::string Expected = "namespace A {\n"
+ "  int y\t = 0;\n"
+ "} // namespace A\n";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 1, 14), 19, ""),
+  createReplacement(getOffset(Code, 2, 3), 3, ""),
+  createReplacement(getOffset(Code, 2, 7), 1, ""),
+  createReplacement(getOffset(Code, 2, 10), 1, ""),
+  createReplacement(getOffset(Code, 2, 12), 2, ""),
+  createReplacement(getOffset(Code, 3, 14), 1, "")});
+
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, RemoveLinesWhenAllNonWhitespaceRemoved) {
+  std::string Code = "struct A {\n"
+ "  A()\n"
+ "  : f(),\n"
+ "g()\n"
+ "  {}\n"
+ "  int f = 0;\n"
+ "  int g = 0;\n"
+ "};";
+  std::string Expected = "struct A {\n"
+ "  A()\n"
+ "  {}\n"
+ "  int f = 0;\n"
+ "  int g = 0;\n"
+ "};";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 3, 5), 3, ""),
+  createReplacement(getOffset(Code, 3, 8), 1, ""),
+  createReplacement(getOffset(Code, 3, 3), 1, ""),
+  createReplacement(getOffset(Code, 4, 5), 3, "")});
+
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, KeepLinesWithInsertsOrReplacesEvenIfBlank) {
+  std::string Code = "struct A {\n"
+ "  A() {}\n"
+ "  int f;\n"
+ "  \n"
+ "  \n"
+ "};";
+  std::string Expected = "struct A {\n"
+ "  A() {}\n"
+ "\n"
+ "\n"
+ "  \n"
+ "\t\n"
+ "};";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 3, 3), 3, "   "),
+  

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 234976.
poelmanc added a comment.

Address most of the feedback, I'll comment individually.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/unittests/clang-change-namespace/ChangeNamespaceTests.cpp
  clang-tools-extra/unittests/clang-move/ClangMoveTests.cpp
  clang/include/clang/Basic/CharInfo.h
  clang/include/clang/Format/Format.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/CleanupTest.cpp

Index: clang/unittests/Format/CleanupTest.cpp
===
--- clang/unittests/Format/CleanupTest.cpp
+++ clang/unittests/Format/CleanupTest.cpp
@@ -349,11 +349,13 @@
  "namespace C {\n"
  "namespace D { int i; }\n"
  "inline namespace E { namespace { int y; } }\n"
- "int x= 0;"
+ "\n"
+ "int x= 0;\n"
  "}";
-  std::string Expected = "\n\nnamespace C {\n"
- "namespace D { int i; }\n\n"
- "int x= 0;"
+  std::string Expected = "\nnamespace C {\n"
+ "namespace D { int i; }\n"
+ "\n"
+ "int x= 0;\n"
  "}";
   tooling::Replacements Replaces =
   toReplacements({createReplacement(getOffset(Code, 3, 3), 6, ""),
@@ -362,6 +364,87 @@
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  std::string Code = "namespace A { // Useless comment\n"
+ "  int x\t = 0;\t\n"
+ "  int y\t = 0;\t\n"
+ "} // namespace A\n";
+  std::string Expected = "namespace A {\n"
+ "  int y\t = 0;\n"
+ "} // namespace A\n";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 1, 14), 19, ""),
+  createReplacement(getOffset(Code, 2, 3), 3, ""),
+  createReplacement(getOffset(Code, 2, 7), 1, ""),
+  createReplacement(getOffset(Code, 2, 10), 1, ""),
+  createReplacement(getOffset(Code, 2, 12), 2, ""),
+  createReplacement(getOffset(Code, 3, 14), 1, "")});
+
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, RemoveLinesWhenAllNonWhitespaceRemoved) {
+  std::string Code = "struct A {\n"
+ "  A()\n"
+ "  : f(),\n"
+ "g()\n"
+ "  {}\n"
+ "  int f = 0;\n"
+ "  int g = 0;\n"
+ "};";
+  std::string Expected = "struct A {\n"
+ "  A()\n"
+ "  {}\n"
+ "  int f = 0;\n"
+ "  int g = 0;\n"
+ "};";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 3, 5), 3, ""),
+  createReplacement(getOffset(Code, 3, 8), 1, ""),
+  createReplacement(getOffset(Code, 3, 3), 1, ""),
+  createReplacement(getOffset(Code, 4, 5), 3, "")});
+
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, KeepLinesWithInsertsOrReplacesEvenIfBlank) {
+  std::string Code = "struct A {\n"
+ "  A() {}\n"
+ "  int f;\n"
+ "  \n"
+ "  \n"
+ "};";
+  std::string Expected = "struct A {\n"
+ "  A() {}\n"
+ "\n"
+ "\n"
+ "  \n"
+ "\t\n"
+ "};";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 3, 3), 3, "   "),
+  createReplacement(getOffset(Code, 3, 7), 2, "  "),
+  createReplacement(getOffset(Code, 3, 1), 0, "\n"),
+  createReplacement(getOffset(Code, 5, 1), 2, "\t")});
+
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, 

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang/include/clang/Basic/CharInfo.h:94
+LLVM_READONLY inline bool isWhitespace(llvm::StringRef S) {
+  for (StringRef::const_iterator I = S.begin(), E = S.end(); I != E; ++I) {
+if (!isWhitespace(*I))

I'd suggest to avoid overloading here. A name like `isWhitespaceOnly` would be 
less ambiguous.

As for implementation, it can be less verbose:
```
for (char C : S)
  if (!isWhitespace(*I))
return false;
return true;
```
or just
```
return llvm::all_of(S, isWhitespace);
```



Comment at: clang/lib/AST/CommentParser.cpp:19
 
-static inline bool isWhitespace(llvm::StringRef S) {
+// Consider moving this useful function to a more general utility location.
+bool isWhitespace(llvm::StringRef S) {

poelmanc wrote:
> kadircet wrote:
> > poelmanc wrote:
> > > gribozavr2 wrote:
> > > > clang/include/clang/Basic/CharInfo.h ?
> > > Done. I renamed it to `isWhitespaceStringRef` to avoid making it an 
> > > overload of the existing `isWhitespace(unsigned char)`, which causes 
> > > ambiguous overload errors at QueryParser.cpp:42 & CommentSema.cpp:237.
> > > 
> > > We could alternatively keep this as an `isWhitespace` overload and 
> > > instead change those two lines to use a `static_cast > > char)>(::isWhitespace)` or precede them with a line like:
> > > ```
> > > bool (*isWhitespaceOverload)(unsigned char) = ::isWhitespace;
> > > ```
> > ah that's unfortunate, I believe it makes sense to have this as an overload 
> > though.
> > 
> > Could you instead make the predicate explicit by putting
> > ```[](char c) { return clang::isWhitespace(c); }```
> > instead of just `clang::isWhitespace` in those call sites?
> Excellent! Better than the two ideas I thought of. Thanks!
I'd like to draw your attention that `isWhitespace(StringRef)` is somewhat 
ambiguous. What exactly does it check? That the string is exactly one 
whitespace character? That it contains a whitespace? That it's only contains 
whitespace? Maybe `isWhitespaceOnly`? See also the comment in CharInfo.h.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682



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

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

`ToolingTests/ApplyAtomicChangesTest.FormatsCorrectLineWhenHeaderIsRemoved` 
also seems to be failing, you can run it with `ninja ToolingTests && 
./tools/clang/unittests/Tooling/ToolingTests 
--gtest_filter="ApplyAtomicChangesTest.FormatsCorrectLineWhenHeaderIsRemoved"`




Comment at: clang/lib/Format/Format.cpp:2385
+  const char *FileText = Code.data();
+  StringRef FilePath; // Must be the same for every Replacement
+  for (const auto  : Replaces) {

maybe assign `FilePath = Replaces.begin()->getFilePath()` here, and early exit 
in the beginning if `Replaces` is empty?
Then you can just `assert(FilePath == R.getFilePath())`



Comment at: clang/lib/Format/Format.cpp:2388
+assert(FilePath.empty() || FilePath == R.getFilePath());
+FilePath = R.getFilePath();
+const int RStartPos = R.getOffset();

no need to re-assign at each loop.



Comment at: clang/lib/Format/Format.cpp:2391
+
+int CurrentRLineStartPos = RStartPos;
+while (CurrentRLineStartPos > 0 &&

it seems like this loop is trying to find line start, would be nice to clarify 
with a comment.

If so seems like the logic is a little buggy, for example:

`int a^;` if the offset is at `^`(namely at `a`) then this will return current 
position as line start, since `a` is preceded by a whitespace.
Maybe change the logic to find the first `\n` before current offset, and skip 
any whitespaces after that `\n` (if really necessary)

btw, please do explain why you are skipping the preceding whitespaces in the 
comment.

could you also add a test case for this one?



Comment at: clang/lib/Format/Format.cpp:2398
+assert(CurrentRLineStartPos >= LineStartPos);
+if (CurrentRLineStartPos != LineStartPos) {
+  // We've moved on to a new line. Wrap up the old one before moving on.

this logic might not work in cases like:

```
in^t a;
int^ b;
```

let's assume you've got replacements at places marked with `^` again, then they 
would have same start position so you would miss the line change event.
maybe count number of `\n`s in the previous loop to detect current line number.

Also please add a test for this case as well.



Comment at: clang/lib/Format/Format.cpp:2416
+ isWhitespace(PriorTextToCheck) &&
+ ReplacementText.empty();
+LineCheckedThroughPos = R.getOffset() + R.getLength();

maybe make it an early skip condition at the beginning of the loop?



Comment at: clang/lib/Format/Format.cpp:2427
+  // (b) the original line was *not* blank.
+  for (const auto  : PotentialWholeLineCuts) {
+const int LineStartPos = LineCheckedThrough.first;

the second loop seems like an over-kill and complicates the code a lot.

IIUC, it is just checking whether any text after replacements is 
"whitespace/newline" and if so deletes the whole line.
Instead of doing all of this, in the previous loop, whenever we see a line 
change could we scan until the EOL, and delete the EOL token if we didn't hit 
anything but whitespaces?

this would mean you won't need to store `PotentialWholeLineCuts`.



Comment at: clang/lib/Format/Format.cpp:2446
+  int CutCount = CutTo - FileText - LineStartPos;
+  llvm::Error Err = NewReplaces.add(
+  tooling::Replacement(FilePath, LineStartPos, CutCount, ""));

nit:
```
if(llvm::Error Err = NewReplaces.add(
  tooling::Replacement(FilePath, LineStartPos, CutCount, "")))
  return std::move(Err);
```



Comment at: clang/unittests/Format/CleanupTest.cpp:368
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  std::string Code = "namespace A { // Useless comment\n"
+ "  int x\t = 0;\t\n"

could you replace these with raw string literals instead to get rid of `\n`s.



Comment at: clang/unittests/Format/CleanupTest.cpp:376
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 1, 14), 19, ""),
+  createReplacement(getOffset(Code, 2, 3), 3, ""),

can you make use of `Annotations` library in 
`llvm/include/llvm/Testing/Support/Annotations.h` for getting offsets into the 
code?

same for the following test cases


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682



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

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked an inline comment as done.
poelmanc added a comment.

In D68682#1753357 , @kadircet wrote:

> also could you rename the revision so that it reflects the fact that, this is 
> a change to clang-format and has nothing to do with clang-tidy ?


Done but feel free to suggest a better title.

(This journey started out with me trying to fix what I thought was a small 
quirk in clang-tidy's readability-redundant-member-init. Great feedback from 
more experienced developers along the way steered me to 
`cleanupAroundReplacements`, which feels perfect, but now I realize that gives 
it a broader impact.)

Thank you so much for the review and feedback!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682



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

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked 5 inline comments as done.
poelmanc added inline comments.



Comment at: clang/lib/AST/CommentParser.cpp:19
 
-static inline bool isWhitespace(llvm::StringRef S) {
+// Consider moving this useful function to a more general utility location.
+bool isWhitespace(llvm::StringRef S) {

kadircet wrote:
> poelmanc wrote:
> > gribozavr2 wrote:
> > > clang/include/clang/Basic/CharInfo.h ?
> > Done. I renamed it to `isWhitespaceStringRef` to avoid making it an 
> > overload of the existing `isWhitespace(unsigned char)`, which causes 
> > ambiguous overload errors at QueryParser.cpp:42 & CommentSema.cpp:237.
> > 
> > We could alternatively keep this as an `isWhitespace` overload and instead 
> > change those two lines to use a `static_cast > char)>(::isWhitespace)` or precede them with a line like:
> > ```
> > bool (*isWhitespaceOverload)(unsigned char) = ::isWhitespace;
> > ```
> ah that's unfortunate, I believe it makes sense to have this as an overload 
> though.
> 
> Could you instead make the predicate explicit by putting
> ```[](char c) { return clang::isWhitespace(c); }```
> instead of just `clang::isWhitespace` in those call sites?
Excellent! Better than the two ideas I thought of. Thanks!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682



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

[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 230375.
poelmanc marked 3 inline comments as done.
poelmanc retitled this revision from "Clang-tidy fix removals removing all 
non-blank text from a line should remove the line" to 
"format::cleanupAroundReplacements removes whole line when Removals leave 
previously non-blank line blank".
poelmanc edited the summary of this revision.
poelmanc added a comment.

I addressed the latest code review comments, added tests to 
`clang/unittests/Format/CleanupTest.cpp`, and updated numerous tests to reflect 
improved removal of blank lines.

I now realize how widely used `cleanupAroundReplacements` is. That's great as 
this fix improves `clang-change-namespace` as a side-effect. However, I have 
little experience running tests beyond clang-tidy or on Linux (I'm testing with 
MSVC) and would appreciate help identifying any test failures or regressions. 
Also please check my change to `FixOnlyAffectedCodeAfterReplacements` in 
clang/unittests/Format/CleanupTest.cpp.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-query/QueryParser.cpp
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/unittests/clang-change-namespace/ChangeNamespaceTests.cpp
  clang-tools-extra/unittests/clang-move/ClangMoveTests.cpp
  clang/include/clang/Basic/CharInfo.h
  clang/include/clang/Format/Format.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp
  clang/lib/AST/CommentSema.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/CleanupTest.cpp

Index: clang/unittests/Format/CleanupTest.cpp
===
--- clang/unittests/Format/CleanupTest.cpp
+++ clang/unittests/Format/CleanupTest.cpp
@@ -349,11 +349,13 @@
  "namespace C {\n"
  "namespace D { int i; }\n"
  "inline namespace E { namespace { int y; } }\n"
- "int x= 0;"
+ "\n"
+ "int x= 0;\n"
  "}";
-  std::string Expected = "\n\nnamespace C {\n"
- "namespace D { int i; }\n\n"
- "int x= 0;"
+  std::string Expected = "\nnamespace C {\n"
+ "namespace D { int i; }\n"
+ "\n"
+ "int x= 0;\n"
  "}";
   tooling::Replacements Replaces =
   toReplacements({createReplacement(getOffset(Code, 3, 3), 6, ""),
@@ -362,6 +364,87 @@
   EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
 }
 
+TEST_F(CleanUpReplacementsTest, RemoveLineWhenAllNonWhitespaceRemoved) {
+  std::string Code = "namespace A { // Useless comment\n"
+ "  int x\t = 0;\t\n"
+ "  int y\t = 0;\t\n"
+ "} // namespace A\n";
+  std::string Expected = "namespace A {\n"
+ "  int y\t = 0;\n"
+ "} // namespace A\n";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 1, 14), 19, ""),
+  createReplacement(getOffset(Code, 2, 3), 3, ""),
+  createReplacement(getOffset(Code, 2, 7), 1, ""),
+  createReplacement(getOffset(Code, 2, 10), 1, ""),
+  createReplacement(getOffset(Code, 2, 12), 2, ""),
+  createReplacement(getOffset(Code, 3, 14), 1, "")});
+
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, RemoveLinesWhenAllNonWhitespaceRemoved) {
+  std::string Code = "struct A {\n"
+ "  A()\n"
+ "  : f(),\n"
+ "g()\n"
+ "  {}\n"
+ "  int f = 0;\n"
+ "  int g = 0;\n"
+ "};";
+  std::string Expected = "struct A {\n"
+ "  A()\n"
+ "  {}\n"
+ "  int f = 0;\n"
+ "  int g = 0;\n"
+ "};";
+  tooling::Replacements Replaces =
+  toReplacements({createReplacement(getOffset(Code, 3, 5), 3, ""),
+  createReplacement(getOffset(Code, 3, 8), 1, ""),
+  createReplacement(getOffset(Code, 3, 3), 1, ""),
+  createReplacement(getOffset(Code, 4, 5), 3, "")});
+
+  EXPECT_EQ(Expected, apply(Code,