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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 21 10:53:53 PST 2023


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



More information about the cfe-commits mailing list