[PATCH] D102488: [clang][deps] Prune unused header search paths

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

The code looks mostly good; some inline comments.

I think it'd be useful to have direct tests for "detecting used search paths". That might be a nice thing to separate out and test independently. E.g., you could add a remark, `-Rused-search-path`, which printed a diagnostic the first time any given search path is used in a TU.

It'd also be really nice to be able to dump this info from the `.pcm` file. I guess it'd be outside the scope of this patch, but I really think there should be a clang-pcm-analyzer where we can put functionality like that. Somewhat useful for more fine-grained testing; super useful as a tool for debugging.



================
Comment at: clang/include/clang/Lex/HeaderSearch.h:295
+  /// Return indices of used search paths.
+  std::set<unsigned> GetUsedSearchPathIdxs() const {
+    std::set<unsigned> UsedSearchPathIdxs;
----------------
I think the word "get" here is a bit misleading, since this isn't a simple accessor, it's a potentially expensive computation. I suggest "compute", unless it's changed to be computed on-the-fly.

I wonder, would llvm::BitVector be more appropriate here, since we expect it to count from 0 and be relatively dense?


================
Comment at: clang/lib/Serialization/ASTReader.cpp:4798-4806
+    case USED_HEADER_SEARCH_PATHS:
+      if (F) {
+        unsigned Idx = 0;
+        unsigned Count = Record[Idx++];
+        for (unsigned I = 0; I < Count; ++I)
+          F->UsedHeaderSearchPathIdxs.insert(Record[Idx++]);
+      }
----------------
If this were stored as a blob it could be lazily parsed; WDYT?

Alternatively, it might be more compact if stored as a bit vector, storing 64-bits positions per element of the record.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102488/new/

https://reviews.llvm.org/D102488