kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1338
+
+ auto ReferencedInclude = convertIncludes(SM, Inc);
+ include_cleaner::walkUsed(
----------------
can we put the rest into a separate function (I know this function is already
quite long, but I think we shouldn't make it worse, and probably refactor
soon). I believe something like below should work?
```
std::vector<Reference> getIncludeUsages(const ParsedAST &AST, const Inclusion
&Include);
```
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1347
+
+ auto Loc = SM.getFileLoc(Ref.RefLocation);
+ for (const auto &H : Providers) {
----------------
nit: no need to calculate the file location, if we're not going to emit a
reference. so maybe move this into the place where we're going to use the `Loc`.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1347
+
+ auto Loc = SM.getFileLoc(Ref.RefLocation);
+ for (const auto &H : Providers) {
----------------
kadircet wrote:
> nit: no need to calculate the file location, if we're not going to emit a
> reference. so maybe move this into the place where we're going to use the
> `Loc`.
this file location might fall outside of the main file when they get expanded
through `#include` directives, so we need something like
https://reviews.llvm.org/D147139
================
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);
----------------
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.
```
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1358
+ auto TokLen =
+ Lexer::MeasureTokenLength(Loc, SM, AST.getLangOpts());
+ Result.Loc.range =
----------------
no need to re-run the lexer, these references are inside the main file for
sure, so you can use token buffer instead.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1381
+ Result.Loc.uri = URIMainFile;
+ Results.References.push_back(std::move(Result));
+ }
----------------
we can return Results directly here, no need to run rest of the logic
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:2004
-// Given a type targeted by the cursor, return one or more types that are more
interesting
-// to target.
-static void unwrapFindType(
- QualType T, const HeuristicResolver* H, llvm::SmallVector<QualType>& Out) {
+// Given a type targeted by the cursor, return one or more types that are more
+// interesting to target.
----------------
can you revert changes below? I feel like they're unrelated formatting changes
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1909
TU.ExtraArgs.push_back("-std=c++20");
+ TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+ #define BAR 5
----------------
rather than introducing these into all tests, can we have our own TestTU and
whatnot in the relevant test? something like MainFileReferencesOnly test
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2322
+ const char *Tests[] = {
+ R"cpp([[#include ^"bar.h"]]
+ int fstBar = [[bar1]]();
----------------
can you also have a second include header that provides some other symbols and
make sure references of those don't get highlighted?
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2329
+
+ R"cpp([[#in^clude <vector>]]
+ std::[[vector]]<int> vec;
----------------
we don't really need rest of these tests, they're testing the "symbol is
provided by first provider included in the main file" logic
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
