VitaNuo added a comment.
Thanks for the comments!
================
Comment at: clang-tools-extra/clangd/Headers.cpp:303
+ llvm::SmallVector<Inclusion> Includes;
+ auto It = MainFileIncludesBySpelling.find(Spelling);
+ if (It == MainFileIncludesBySpelling.end())
----------------
kadircet wrote:
> nit:
> ```
> llvm::SmallVector<Inclusion*> Includes;
> for (auto Idx : MainFileIncludesBySpelling.lookup(Spelling))
> Includes.push_back(&MainFileIncludes[Idx]);
> return Includes;
> ```
But `lookup` will return 0 if it doesn't find the spelling in the map (default
value of `int`). And at the same time 0 is a valid index. Isn't that just wrong?
================
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:
> 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?
Sure, thanks. No particular reason.
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
