This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement cross reference request for #include lines.
ClosedPublic

Authored by VitaNuo on Mar 28 2023, 5:44 AM.

Diff Detail

Event Timeline

VitaNuo created this revision.Mar 28 2023, 5:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2023, 5:44 AM
VitaNuo requested review of this revision.Mar 28 2023, 5:44 AM
VitaNuo updated this revision to Diff 508975.Mar 28 2023, 5:53 AM

Smaller improvements.

VitaNuo updated this revision to Diff 508986.Mar 28 2023, 6:40 AM

Formatting.

VitaNuo updated this revision to Diff 509015.Mar 28 2023, 7:53 AM

Remove redundant code.

VitaNuo updated this revision to Diff 509017.Mar 28 2023, 7:55 AM

Formatting.

kadircet added inline comments.Mar 30 2023, 5:29 AM
clang-tools-extra/clangd/XRefs.cpp
1396

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);
1405

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.

1405

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

1406

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.
1416

no need to re-run the lexer, these references are inside the main file for sure, so you can use token buffer instead.

1439

we can return Results directly here, no need to run rest of the logic

2020

can you revert changes below? I feel like they're unrelated formatting changes

clang-tools-extra/clangd/unittests/XRefsTests.cpp
1908

rather than introducing these into all tests, can we have our own TestTU and whatnot in the relevant test? something like MainFileReferencesOnly test

2302

can you also have a second include header that provides some other symbols and make sure references of those don't get highlighted?

2309

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

VitaNuo updated this revision to Diff 510027.Mar 31 2023, 7:30 AM
VitaNuo marked 10 inline comments as done.

Address review comments.

VitaNuo updated this revision to Diff 510032.Mar 31 2023, 7:33 AM

Simplify.

VitaNuo updated this revision to Diff 510036.Mar 31 2023, 7:38 AM

Remove extra formatting changes.

VitaNuo updated this revision to Diff 510039.Mar 31 2023, 7:44 AM

Rename function.

Thanks for the comments!

clang-tools-extra/clangd/XRefs.cpp
1406

Is the comment under the code in the first snippet a mistake/outdated content? It confused me.

VitaNuo updated this revision to Diff 510042.Mar 31 2023, 7:59 AM

Simplify.

kadircet added inline comments.Apr 3 2023, 5:29 AM
clang-tools-extra/clangd/IncludeCleaner.cpp
455

return std::nullopt

clang-tools-extra/clangd/IncludeCleaner.h
87

can you also move tests related to this functionality from HoverTests into IncludeCleanerTests ?

clang-tools-extra/clangd/XRefs.cpp
1378

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.

1382

const auto &Includes = AST.getIncludeStructure().MainFileIncludes; (& is the important bit, as we're copying all of them otherwise)

1383

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;
...
1406

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 Includes matched by the first provider, rather than returning the provider. but feel free to keep it as-is.

VitaNuo updated this revision to Diff 510484.Apr 3 2023, 6:39 AM
VitaNuo marked 5 inline comments as done.

Address review comments.

Thanks for the comments!

clang-tools-extra/clangd/IncludeCleaner.h
87

Not really, not sure what you mean.

This function receives a ranked list of providers that should be a result of include_cleaner analysis.

If I just test the function based on a fake list of providers, the test will be trivial. The current hover test seems to provide more value than that.

kadircet added inline comments.Apr 18 2023, 1:38 AM
clang-tools-extra/clangd/IncludeCleaner.h
87

If I just test the function based on a fake list of providers, the test will be trivial.

but that's the "unit" of logic we're trying to test, no?

VitaNuo updated this revision to Diff 514901.Apr 19 2023, 4:23 AM

Extract a test for th firstMatchedProvider method.

VitaNuo marked an inline comment as done.Apr 19 2023, 4:24 AM
VitaNuo added inline comments.
clang-tools-extra/clangd/IncludeCleaner.h
87

Ok, I've extracted a test. Hopefully that's what you've meant.

kadircet accepted this revision.Apr 19 2023, 5:09 AM

thanks!

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
398

can you add a similar test case in which foo.h comes before bar.h ?

410

nit: no need for the filename

This revision is now accepted and ready to land.Apr 19 2023, 5:09 AM
VitaNuo updated this revision to Diff 515227.Apr 20 2023, 12:09 AM
VitaNuo marked 3 inline comments as done.

Add a test case.

This revision was landed with ongoing or failed builds.Apr 20 2023, 12:13 AM
This revision was automatically updated to reflect the committed changes.