[PATCH] D117461: [clangd] IncludeCleaner: Attach "insert prgama keep" fix-it to diagnostic

[Sam McCall via Phabricator via cfe-commits]

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