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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 18 14:55:42 PST 2022

sammccall added a comment.


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 \

(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?

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list