[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank
Kadir Cetinkaya via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 3 04:37:21 PST 2020
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 &NewReplaces,
+ 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 &NewReplaces,
+ 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 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.
ah thanks, I somehow missed the `vertical` part.
================
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.
----------------
poelmanc wrote:
> 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.
nvm this one, it was a consequence of missing `vertical`
================
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"
----------------
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.
================
Comment at: clang/unittests/Format/CleanupTest.cpp:376
+ tooling::Replacements Replaces =
+ toReplacements({createReplacement(getOffset(Code, 1, 14), 19, ""),
+ createReplacement(getOffset(Code, 2, 3), 3, ""),
----------------
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.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68682/new/
https://reviews.llvm.org/D68682
More information about the cfe-commits
mailing list