[PATCH] D120306: [clangd] IncludeCleaner: Add support for IWYU pragma private

[Sam McCall via Phabricator via cfe-commits]

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