[PATCH] D141670: [include-cleaner] FindHeaders respects IWYU export pragma for standard headers.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks!



================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:192
     int HashLine = SM.getLineNumber(HashFID, SM.getFileOffset(HashLoc));
-    checkForExport(HashFID, HashLine, File ? &File->getFileEntry() : nullptr);
+    Header IncludedHeader = File ? &File->getFileEntry() : nullptr;
+    if (IsAngled)
----------------
nit: I'd make this an optional instead (as the code in checkForExport looks weird now, we do nullptr check on one branch but not the other) and change the flow to:
```
std::optional<Header> IncludedHeader;
if(IsAngled) {
  if(stdlibheader) ...
}

if(!IncludedHeader && File) {
 ...
}
```


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:212
         Top.SeenAtLine == HashLine) {
-      if (IncludedHeader)
-        Out->IWYUExportBy[IncludedHeader->getUniqueID()].push_back(
-            Top.Path);
+      if (IncludedHeader.kind() == Header::Standard)
+        Out->StdIWYUExportBy[IncludedHeader.standard()].push_back(Top.Path);
----------------
nit:
```
switch(IncludedHeader.kind()) {
case Verbatim:
  assert(false && "...");
  break;
case others:
...
}
```

to be consistent with rest of the places (and more explicit about handling of verbatim)


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:122
+  tooling::stdlib::Symbol StdString =
+      *tooling::stdlib::Symbol::named("std::", "string");
+  EXPECT_THAT(
----------------
hokein wrote:
> kadircet wrote:
> > nit: `tooling::stdlib::Header::named("<string>")`
> We need this `tooling::stdlib::Symbol` here, `include_cleaner::findHeaders` accepts a `SymbolLocation`, `SymbolLocation` is construct from a `tooling::stdlib::Symbol`.
oops you're right


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141670