This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement textDocument/codeLens
Needs ReviewPublic

Authored by Trass3r on Aug 5 2022, 2:26 PM.
Tokens
"Like" token, awarded by SR_team.

Details

Reviewers
nridge
kadircet
Summary

Co-authored-by: lightmelodies <lightmelodies@outlook.com>

Diff Detail

Event Timeline

Trass3r created this revision.Aug 5 2022, 2:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 2:26 PM
Trass3r edited the summary of this revision. (Show Details)Aug 5 2022, 2:33 PM
Trass3r edited projects, added Restricted Project; removed Restricted Project.
Trass3r edited subscribers, added: sammccall; removed: mgorny, javed.absar, arphaman, usaxena95.
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 2:33 PM
Trass3r edited the summary of this revision. (Show Details)Aug 5 2022, 2:37 PM
Trass3r added a subscriber: lightmelodies.
Trass3r updated this revision to Diff 450421.Aug 5 2022, 2:51 PM

"base" refs are only useful on class forward decls
but for some reasons this still shows up on dtor implementations

Trass3r updated this revision to Diff 450422.Aug 5 2022, 2:55 PM

squashed

Trass3r added inline comments.Aug 5 2022, 2:59 PM
clang-tools-extra/clangd/CodeLens.cpp
86

they still show up on dtor implementations iirc

Trass3r published this revision for review.Aug 5 2022, 2:59 PM
Trass3r added a comment.EditedAug 5 2022, 3:04 PM

I just searched again and found https://reviews.llvm.org/D91930.
But seems like it never got updated with the latest code featured here. Though looks like the test got dropped.

Haven't had a chance to look at this yet. I do see that the earier implementation in D91930 was the subject of some design discussions about performance with @kadircet, adding him as an additional reviewer.

Trass3r updated this revision to Diff 473662.EditedNov 7 2022, 7:24 AM
Trass3r edited the summary of this revision. (Show Details)

remove bases codelens for classes
fix lit test
exclude self from ref count

https://github.com/clangd/clangd/issues/442#issuecomment-1125056812

Trass3r added inline comments.Nov 7 2022, 8:55 AM
clang-tools-extra/clangd/CodeLens.cpp
93

As noted in the previous PR it may be of low value, I guess only if override is missing or if it actually hides several functions.

SR_team added a subscriber: SR_team.
Trass3r added inline comments.Nov 13 2022, 2:51 PM
clang-tools-extra/clangd/tool/ClangdMain.cpp
348 ↗(On Diff #473662)

I guess we should rather make it opt-in and gather some feedback.

One remaining issue is multiple lenses for template code like

template <int V>
int i = 0;

template int i<0>;
template int i<1>;
template int i<2>;

template <int V>
struct Foo {
    int foo(); // I see 3 codelenses here
};

template struct Foo<0>;
template struct Foo<1>;

int main()
{
    // return Foo<0>().foo() + Foo<1>().foo();
    // return i<0> + i<1> + i<2>;
}

One remaining issue is multiple lenses for template code like

template <int V>
int i = 0;

template int i<0>;
template int i<1>;
template int i<2>;

template <int V>
struct Foo {
    int foo(); // I see 3 codelenses here
};

template struct Foo<0>;
template struct Foo<1>;

int main()
{
    // return Foo<0>().foo() + Foo<1>().foo();
    // return i<0> + i<1> + i<2>;
}

Well, multiple lenses occured due to https://github.com/lightmelodies/llvm-project/blob/codelens/clang-tools-extra/clangd/FindSymbols.cpp#L500
And can be fixed by https://github.com/lightmelodies/llvm-project/blob/codelens/clang-tools-extra/clangd/CodeLens.cpp#L69

Trass3r added a comment.EditedNov 17 2022, 2:18 PM

Thanks, it seems to fix the base case, but I still see multiple lenses when I add

template <int V>
int Foo<V>::foo()
{
    return 0;
}

Thanks, it seems to fix the base case, but I still see multiple lenses when I add

template <int V>
int Foo<V>::foo()
{
    return 0;
}

Interesting, I see the same issue with DocumentSymbols. Maybe because foo is explicit specialization (though outside Foo<1>) so both check are passed. However I can not find a way to detect such case right now.