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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits