[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 &R : 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 &LineCheckedThrough : 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