hokein added inline comments.
================ Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:76 + /// Returns true if the given file is marked with the IWYU private pragma + /// without public mapping. + bool isPrivate(const FileEntry *File) const; ---------------- I think this function should return true if the given file is marked with the IWYU private mapping pragma, because conceptually these file are also private. ================ Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:95 + /// Contains all files marked with IWYU private pragmas that + /// do not have public header mapping. This uses the UniqueID similar to + /// the IWYU private pragmas with public mapping. ---------------- It should collect files with mapping IWYU private pragmas. and I think the last sentence can be removed (the intention of using UniqueID is clear from other comments). ================ Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:266 } + } else if (Pragma->startswith("private")) { + Out->IWYUPrivate.insert(SM.getFileEntryForID(CommentFID)->getUniqueID()); ---------------- I'd suggest moving this to the above line 236 where we handle the private mapping. ================ Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:372 + Inputs.ExtraFiles["public.h"] = R"cpp( + #include "private.h"; + #include "private2.h"; ---------------- no extra `;` token needed for the `#include`. ================ Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:393 + assert(Private2FE); + EXPECT_TRUE(PI.isPrivate(Private2FE.get())); } ---------------- add `EXPECT_TRUE(PI.isPrivate(PrivateFE.get()))`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138678/new/ https://reviews.llvm.org/D138678 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits