Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang-tools-extra/include-cleaner/lib/Record.cpp | ||
|---|---|---|
| 206 | just saying StandardHeader here isn't really useful. what about changing signautre here to take a include_cleaner::Header instead, named IncludedHeader and switch over kind to add it to ExportBy map? I don't think there's any value in storing Exporters for <vector> both as a physical entry and a stdlib entry. | |
| clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp | ||
| 122 | nit: tooling::stdlib::Header::named("<string>") | |
| clang-tools-extra/include-cleaner/unittests/RecordTest.cpp | ||
| 437 | drop the comment Line 1 | |
address comments
| clang-tools-extra/include-cleaner/lib/Record.cpp | ||
|---|---|---|
| 206 | Ah, nice idea! thanks. | |
| clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp | ||
| 122 | We need this tooling::stdlib::Symbol here, include_cleaner::findHeaders accepts a SymbolLocation, SymbolLocation is construct from a tooling::stdlib::Symbol. | |
thanks!
| clang-tools-extra/include-cleaner/lib/Record.cpp | ||
|---|---|---|
| 192–200 | 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) {
 ...
} | |
| 214–227 | 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) | |
| clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp | ||
| 122 | oops you're right | |
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) { ... }