[PATCH] D143509: Move the BySpelling map to IncludeStructure.

[Viktoriia Bakalova via Phabricator via cfe-commits]

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