[PATCH] D136071: [include-cleaner] WIP: Add PragmaIncludes which handles include-mapping pragmas.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

LG from my side



================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:58
+  // Line numbers for the include directives in the main file that have the
+  // `IWYU pragma: keep` right after.
+  llvm::DenseSet</*0-indexed line number*/ unsigned> ShouldKeep;
----------------
kadircet wrote:
> we should also keep headers that're marked as `export`.
export isn't in this patch.
I don't think the details of where this data comes belongs in the header, it's not important to either the data structure or the interface.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:67
+  llvm::DenseMap<llvm::sys::fs::UniqueID, std::string /*VerbatimSpelling*/>
+      IWYUPrivate;
+
----------------
nit: naming a member after the keys rather than the values is confusing, consider `IWYUPublic` or just `PublicMapping`?


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:112
+
+bool PragmaIncludes::shouldKeep(unsigned HashLineNumber) const {
+  return ShouldKeep.find(HashLineNumber) != ShouldKeep.end();
----------------
nit: we can inline these trivial implementations into the header


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136071