kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/Headers.cpp:303
+ llvm::SmallVector<Inclusion> Includes;
+ auto It = MainFileIncludesBySpelling.find(Spelling);
+ if (It == MainFileIncludesBySpelling.end())
----------------
nit:
```
llvm::SmallVector<Inclusion*> Includes;
for (auto Idx : MainFileIncludesBySpelling.lookup(Spelling))
Includes.push_back(&MainFileIncludes[Idx]);
return Includes;
```
================
Comment at: clang-tools-extra/clangd/Headers.h:167
+ // Spelling should include brackets or quotes, e.g. <foo>.
+ llvm::SmallVector<HeaderID>
+ mainFileIncludesWithSpelling(llvm::StringRef Spelling) const {
----------------
kadircet wrote:
> we're still returning just the `HeaderID`, the suggestion was to return
> `llvm::SmallVector<Inclusion*>` so that applications can work with other
> information available in the `Inclusion`. we also won't have any requirements
> around include being resolved that way. the application should figure out
> what to do if `HeaderID` is missing.
>
> also can you move this function body to source file instead?
almost, but not right. we're returning copies of `Inclusion`s here, not
pointers to `Inclusion`s inside the main file. the suggested signature was
`llvm::SmallVector<Inclusion*>` any reason for returning by value?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:516
+ Includes.mainFileIncludesWithSpelling(H.verbatim())) {
+ std::optional<unsigned int> HeaderOpt = Inc.HeaderID;
+ if (HeaderOpt.has_value()) {
----------------
nit:
```
if(!Inc.HeaderID.has_value())
continue;
Used.insert(static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID));
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
