kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:443
+ // No match for this provider in the includes list.
+ return {};
+}
----------------
`return std::nullopt`
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:87
+std::optional<include_cleaner::Header>
+firstMatchedProvider(const include_cleaner::Includes &Includes,
+ llvm::ArrayRef<include_cleaner::Header> Providers);
----------------
can you also move tests related to this functionality from HoverTests into
IncludeCleanerTests ?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1320
+ReferencesResult maybeFindIncludeReferences(ParsedAST &AST, Position Pos,
+ URIForFile URIMainFile) {
----------------
can you put this into anon namespace?
we should also change the return type to `std::optional<ReferencesResult>`, as
we can "legitimately" have empty result for an include, e.g. it's unused or all
of the refs are implicit etc.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1324
+ const SourceManager &SM = AST.getSourceManager();
+ auto Includes = AST.getIncludeStructure().MainFileIncludes;
+ auto ConvertedMainFileIncludes = convertIncludes(SM, Includes);
----------------
`const auto &Includes = AST.getIncludeStructure().MainFileIncludes;` (`&` is
the important bit, as we're copying all of them otherwise)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1325
+ auto Includes = AST.getIncludeStructure().MainFileIncludes;
+ auto ConvertedMainFileIncludes = convertIncludes(SM, Includes);
+ for (auto &Inc : Includes) {
----------------
let's move this into the for loop, after we've found a matching include to not
necessarily create a copy of all the includes again.
nit: you can also write the loop below as following and reduce some nesting.
```
auto IncludeOnLine = llvm::find_if(Includes, [&Pos](const Inclusion& Inc) {
return Inc.HashLine == Pos.Line; });
if (IncludeOnLine == Includes.end())
return Results;
const auto &Inc = *IncludeOnLine;
...
```
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1348
+ auto Loc = SM.getFileLoc(Ref.RefLocation);
+ for (const auto &H : Providers) {
+ auto MatchingIncludes = ConvertedMainFileIncludes.match(H);
----------------
VitaNuo wrote:
> kadircet wrote:
> > we're implementing and testing this logic twice now, once in here and once
> > in hover. can we instead have a helper in `IncludeCleaner.h` that looks
> > like:
> > ```
> > std::optional<include_cleaner::Header> firstSatisfiedProvider(const
> > include_cleaner::Includes& Includes,
> > llvm::ArrayRef<include_cleaner::Header> Providers);
> > // I'd actually return the matching `std::vector<const Include*>` (the
> > highest ranked provider that matched some includes in main file), and check
> > if the include of interest is part of that set for rest of the operations.
> > // Since it represents both the provider and the include in the main file.
> > whereas the provider on it's own doesn't say anything about which include
> > in main file triggered satisfaction.
> > ```
> > and turn these call sites into
> > ```
> > auto Provider = firstSatisfiedProvider(ConvertedMainFileIncludes,
> > Providers);
> > if(!Provider || ReferencedInclude.match(Provider).empty())
> > return;
> > // Include in question provides the symbol, do magic.
> > ```
> Is the comment under the code in the first snippet a mistake/outdated
> content? It confused me.
that wasn't supposed to be a comment **in** the code but rather a comment
**for** the code :D i was suggesting to return the set of `Include`s matched by
the first provider, rather than returning the provider. but feel free to keep
it as-is.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147044/new/
https://reviews.llvm.org/D147044
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
