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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked an inline comment as done.
VitaNuo added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:237
+      auto *FE = SM.getFileEntryForID(SM.getFileID(Range.getBegin()));
+      if (!FE) {
+        return false;
----------------
kadircet wrote:
> nit: you can drop the braces here. LLVM convention is to not have braces when a condition/loop body has only a single statement.
Ah ok, thank you. It's a bit funny, my whole life until now I've been rigorously taught to never do this ;-)


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:240
+      }
+      if (Pragma->consume_front(", include ")) {
+        // We always insert using the spelling from the pragma.
----------------
kadircet wrote:
> nit: you can also rewrite this as:
> ```
> StringRef PublicHeader;
> if (Pragma->consume_front(", include ")) {
>   PublicHeader = save(Pragma->startswith("<") || Pragma->startswith("\"")
>                       ? (*Pragma)
>                       : ("\"" + *Pragma + "\"").str());
> }
> Out->IWYUPublic.insert({FE->getLastRef().getUniqueID(), PublicHeader});
> ```
Yeah this is good, thanks.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:381
+    // IWYU pragma: private
+    class Private {};
+  )cpp";
----------------
kadircet wrote:
> VitaNuo wrote:
> > 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?
> it means `translation unit`, it's the source file provided to the compiler after all preprocessor directives are processed.
Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138678