Page MenuHomePhabricator

[clangd] Suggest code-completions for overriding base class virtual methods.
ClosedPublic

Authored by kadircet on Aug 17 2018, 7:21 AM.

Details

Summary

Whenever a code-completion is triggered within a class/struct/union looks at
base classes and figures out non-overriden virtual functions. Than suggests
completions for those.

Diff Detail

Event Timeline

kadircet created this revision.Aug 17 2018, 7:21 AM
kadircet retitled this revision from Suggest code-completions for overriding base class virtual methods. to [clangd] Suggest code-completions for overriding base class virtual methods..Aug 17 2018, 8:36 AM

I think it is reasonable to make Sema support suggesting override methods, instead of implementing it in clangd side?

clangd/CodeComplete.cpp
206

nit: CR->methods().

210

nit: we can simplify the code like Overrides[Name].push_back(Method).

219

I think we should check whether BR == nullptr here.

1268

It seems that we treat it as a special case, the code path here runs out of the ranking path.

kadircet updated this revision to Diff 161683.Aug 21 2018, 3:43 AM
kadircet marked 3 inline comments as done.
  • Put overrides through scoring and resolve nits.
kadircet marked an inline comment as done.Aug 21 2018, 3:44 AM
kadircet added inline comments.
clangd/CodeComplete.cpp
1268

Put override suggestions to ranking system.

kadircet marked an inline comment as done.Aug 21 2018, 3:44 AM

I think it is reasonable to make Sema support suggesting override methods, instead of implementing it in clangd side?

drive-by: +1 to this.

After today's offline discussion I suppose we are not going to implement it within Sema. And also I think getVirtualNonOverridenMethods is a pretty generic function that has no ties to clangd, so it can be easily moved into Sema. Should we still move it into a separate file or leave it there?

Looks good mostly, a few nits. We should make sure all related comments are updated accordingly

clangd/CodeComplete.cpp
198

Since we are returning CodeCompletionResult, maybe naming it like getNonOverridenCompleteionResults is clearer?

222

nit: Can't we use Overrides.find(Method->getName()) and the other places as well?

224

nit: use {} around the body of if -- the one-line for statement is across line, adding {} around it will improve the readability.

1258–1259

nit: we need to update the comment accordingly.

1301–1304

here as well.

1345

IIUC, we are treating the override results the same SemaResult, it is safe as long as Sema doesn't provide these overrides in its code completion results, otherwise we will have duplicated completion items?

I think we probably need a proper comment explaining what's going on here.

kadircet updated this revision to Diff 161963.Aug 22 2018, 8:12 AM
kadircet marked 6 inline comments as done.
  • Resolve discussions.
  • Fix a bug inside summarizeOverride.
hokein accepted this revision.Aug 23 2018, 1:16 AM

Thanks, LGTM.

clangd/CodeComplete.cpp
210

nit: here as well, use Overrides[Method->getName()].push_back...

This revision is now accepted and ready to land.Aug 23 2018, 1:16 AM
kadircet updated this revision to Diff 162151.Aug 23 2018, 5:27 AM
  • Resolve discussions.
kadircet updated this revision to Diff 162157.Aug 23 2018, 6:03 AM
  • Resolve discussions.
  • Move override generation logic from render to item generation so that internal clients can use it as well, also use a boolean for checking override status of a bundle to be more efficient.
  • Change unittests accordingly, get rid of unused IsOverride field.
hokein accepted this revision.Aug 23 2018, 6:10 AM

still LG.

This revision was automatically updated to reflect the committed changes.