This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kbobyrev on Jan 17 2022, 2:29 AM.

Details

Reviewers
sammccall
Summary

Now that IWYU pragma: keep supresses the warning for unused header, it should
be possible to suggest the user both removing the header and adding a pragma to
keep it instead.

Diff Detail

Event Timeline

kbobyrev created this revision.Jan 17 2022, 2:29 AM
kbobyrev requested review of this revision.Jan 17 2022, 2:29 AM

Nice!

clang-tools-extra/clangd/IncludeCleaner.cpp
212

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(),
  };
}
223

this is incorrect for

#include \
  <foo.h>

(Happy if it works or rejects this case, but it shouldn't silently do the wrong thing.)

227

I'd expect error cases to be handled somehow, the fact that this function always succeeds is suspicious

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
1616–1618

nit: insert->pragma_keep?

kbobyrev updated this revision to Diff 405238.Feb 2 2022, 5:54 AM
kbobyrev marked 4 inline comments as done.

Address review comments.

kbobyrev abandoned this revision.May 19 2022, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 5:52 AM