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