[PATCH] D138678: [include-cleaner] Capture private headers in PragmaIncludes.

[Haojian Wu via Phabricator via cfe-commits]

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