[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 &NewReplaces,
+                                 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<tooling::Replacement>`. 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
> > `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`.
> This one is not a style choice. Annotations was basically not available at the time the other tests were written, and it makes the tests a lot more readable.
> there's no easy way to figure out what is meant by all those offsets entered as numbers. So please make use of them in here, and I would really appreciate if you have time to send a follow-up patch to convert the rest of the tests in the file.
Thanks, I figured out the Annotations pretty quickly and it definitely is easier to read and reason about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68682