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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
