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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked 4 inline comments as done.
VitaNuo added a comment.

The formatting has introduced a bit of unrelated diff. It's not much, so I hope that's fine.



================
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.
----------------
hokein wrote:
> 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).
I've followed Kadir's suggestion below now and merged the private pragmas with the IWYUPublic map.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:97
+  /// the IWYU private pragmas with public mapping.
+  llvm::DenseSet<llvm::sys::fs::UniqueID> IWYUPrivate;
+
----------------
kadircet wrote:
> i'd actually merge this with the previous map, and store empty verbatim spellings. semantics of `getPublic` is to return empty path when there are no mappings anyway.
Sounds reasonable. I wasn't sure which of the options to pick in the first place.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:381
+    // IWYU pragma: private
+    class Private {};
+  )cpp";
----------------
kadircet wrote:
> nit: no need for the declaration of `Private` here (nor in `private.h`). it's actually introducing a double definition error into TU now.
Out of curiosity: what's TU?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138678