Page MenuHomePhabricator

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

Authored by ilya-golovenko on Wed, Jul 29, 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.Wed, Jul 29, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 29, 5:41 AM
ilya-golovenko requested review of this revision.Wed, Jul 29, 5:41 AM

Trigger new build

kadircet added inline comments.Wed, Jul 29, 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.Wed, Jul 29, 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.Sat, Aug 1, 10:19 AM

thanks, lgtm!

let me know if i should land this for you

This revision is now accepted and ready to land.Sat, Aug 1, 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>