This is an archive of the discontinued LLVM Phabricator instance.

Add document outline symbols from unnamed contexts, e.g. extern "C".
ClosedPublic

Authored by ilya-golovenko on Jul 29 2020, 5:41 AM.

Details

Summary

It is necessary to traverse children of unnamed declaration contexts
to get symbols which are currently missing in document outline, e.g.:

extern "C" {
void foo();
}

Diff Detail

Event Timeline

ilya-golovenko created this revision.Jul 29 2020, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2020, 5:41 AM
ilya-golovenko requested review of this revision.Jul 29 2020, 5:41 AM

Trigger new build

kadircet added inline comments.Jul 29 2020, 5:57 AM
clang-tools-extra/clangd/FindSymbols.cpp
198–199

this will result in traversal of other declcontexts like block/capturedecls.

i think we should rather change the VisitKind to {No, Decl, Children} and then make shouldVisit return a VisitKindSet and take a Decl* instead of a NamedDecl.
Later on we should first check for LinkageSpecDecl and return Children only while keeping the rest of the logic the same (i.e. try to cast to nameddecl and return No if it fails.
Finally logic in here should also be adjusted accordingly to visit children and the decl itself separately.

Do you have other cases apart from extern'd symbols?

ilya-golovenko added inline comments.Jul 29 2020, 7:07 AM
clang-tools-extra/clangd/FindSymbols.cpp
198–199

I have only C++20 ExportDecl in mind. Initially I wanted to make changes similar like you propose, but tried to minimize the diff :-) I will re-work the fix according to your proposal.

Changes afte code review, add test for export context

@kadircet I decided to add OnlyChildren to enum VisitKind instead of making a VisitKindSet - this helped to minimize the changes in shouldVisit(). What do you think?

Remove invalid comment

@kadircet I decided to add OnlyChildren to enum VisitKind instead of making a VisitKindSet - this helped to minimize the changes in shouldVisit(). What do you think?

SGTM.

clang-tools-extra/clangd/FindSymbols.cpp
211–217

ah this is complicated now, but at least for the sake of nestedness:

if (Visit == OnlyChildren)
  return traverseChildren;

// We must have the Decl in the result set.
auto *ND = llvm::cast<NamedDecl>(D);
auto Sym = declToSym(AST.getASTContext(), *ND);
if (!Sym) return;
Results.push_back(std::move(*Sym));

if(Visit == OnlyDecl) return;

assert(Visit == DeclAndChildren && "Unexpected VisitKind");
traverseChildren(Result.back()->children);

Simplify logic in traverseDecl method

kadircet accepted this revision.Aug 1 2020, 10:19 AM

thanks, lgtm!

let me know if i should land this for you

This revision is now accepted and ready to land.Aug 1 2020, 10:19 AM

thanks, lgtm!

let me know if i should land this for you

Thank you much for the review.

I don't have commit access so could you please land it for me:
Ilya Golovenko <ilya.golovenko@huawei.com>