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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
