sammccall added a comment.
Thanks, this is much nicer!
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:310
findReferencedFiles(const ReferencedLocations &Locs, const SourceManager &SM,
- llvm::function_ref<FileID(FileID)> HeaderResponsible) {
+ llvm::function_ref<void(FileID, llvm::DenseSet<FileID> &,
+ llvm::StringSet<> &)>
----------------
this signature/hook is a bit hard to understand (and undocumented).
I think understanding at least the idea of an IWYU mapping (if not the syntax)
is in scope for this library, and I think it would be clearer to have a
function<Optional<StringRef>(FileID)> specifically to look up those mappings
instead
(It's fine that clangd uses the existing CanonicalIncludes mapping for now, but
I also think when we pull this out as a library, that library will provide some
facility for recording these mappings, so the interface here shouldn't be too
high-level)
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:62
+ /// in private headers (private headers have IWYU pragma: private, include
+ /// "public.h"). We store spelling of the public header files here to avoid
+ /// dealing with full filenames and visibility.
----------------
Specify whether the spelling includes quotes or not
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:64
+ /// dealing with full filenames and visibility.
+ llvm::StringSet<> PublicHeaders;
};
----------------
Maybe call this ExactSpellings or SpelledUmbrellas? I think public vs private
isn't the right distinction, stdlib headers and user headers are also often
public.
================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:1640
TEST(DiagnosticsTest, IncludeCleaner) {
Annotations Test(R"cpp(
----------------
Must we test this here (which is AIUI a smoke test of emitting diagnostics)
instead of in the getUnused tests in IncludeCleanerTests?
================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:1690
+ withFix(Fix(Test.range("fix"), "", "remove #include
directive"))),
+ AllOf(Diag(Test.range("diag_private"),
+ "included header private.h is not used"),
----------------
This doesn't seem strictly correct (I assume it fires even if we don't include
public.h).
There's a problem here, but it isn't that the header is unused. And worse, at
the moment we can't help you fix this problem, and the suggested change here is
likely to break the build.
For unused, I think we probably want to mark both as used?
(For missing, we'd only require the public header)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120306/new/
https://reviews.llvm.org/D120306
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
