sammccall added a comment.
Nice!
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:212
+ clangd::Range Result;
+ Result.end = Result.start = offsetToPosition(Code, HashOffset);
+
----------------
I think we should be a bit more direct about the *content* that we're matching
here, rather than the coordinates.
e.g. the current logic is broken in various ways if the directive is split over
physical lines, and dealing with coordinates makes it both hard to fix and
detect.
And it seems like you're going to offer a fix erasing any existing trailing
comment.
Possibly there are other unexpected patterns where the code might get mangled
too.
Maybe more like:
```
StringRef RestOfLine = Code.substr(HashOffset).take_until('\n' || '\r');
if (!RestOfLine.consume_front("#"))
return fail;
RestOfLine = RestOfLine.ltrim();
// and consume the rest of the stuff we expect on the line
if (RestOfLine.ltrim().empty()) {
return {
offsetToPosition(Code, RestOfLine.begin() - Code.data(),
offsetToPosition(Code, RestOfLine.end() - Code.data(),
};
}
```
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:223
+ 1;
+ Result.end.character +=
+ lspLength(Code.drop_front(HashOffset).take_until([](char C) {
----------------
this is incorrect for
```
#include \
<foo.h>
```
(Happy if it works or rejects this case, but it shouldn't silently do the wrong
thing.)
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:227
+ }));
+ return Result;
+}
----------------
I'd expect error cases to be handled somehow, the fact that this function
always succeeds is suspicious
================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:1616
Annotations Test(R"cpp(
-$fix[[ $diag[[#include "unused.h"]]
+$remove[[ $diag[[#include "unused.h"]]$insert[[]]
]]
----------------
nit: insert->pragma_keep?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117461/new/
https://reviews.llvm.org/D117461
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
