This is an archive of the discontinued LLVM Phabricator instance.

[CodeCompletion] Enable signature help when initializing class/struct/union members.
ClosedPublic

Authored by kadircet on Sep 11 2018, 2:19 AM.

Details

Summary

Factors out member decleration gathering and uses it in parsing to call signature
help. Doesn't support signature help for base class constructors, the code was too
coupled with diagnostic handling, but still can be factored out but just needs
more afford.

Diff Detail

Repository
rC Clang

Event Timeline

kadircet created this revision.Sep 11 2018, 2:19 AM
ilya-biryukov added inline comments.Sep 11 2018, 2:43 AM
include/clang/Sema/Sema.h
10808

The name is very generic, but the helper is only applicable to our specific case (looking up ctor member initializer). Maybe choose a different name?
Something like tryLookupCtorInitMemberDecl would be closer to what it actually does.

lib/Parse/ParseDeclCXX.cpp
3471

We're calling completion in other instances, this gives us valuable information (preferred type).
Why not do the same in this case?

3473

That's a lot of code in parser.

We could probably extract this as a helper in Sema (similar to ProduceConstructorSignatureHelp) to follow the same pattern that we have for all the other signature help calls.
Would keep the parser simpler and, as an added benefit, would allow to make tryGetMember helper private (as it should be).
WDYT?

kadircet updated this revision to Diff 164844.Sep 11 2018, 4:34 AM
kadircet marked 3 inline comments as done.
  • Move ValueDecl extraction to a helper.
  • Call completion handlers as well.
ilya-biryukov added inline comments.Sep 11 2018, 4:46 AM
lib/Parse/ParseDeclCXX.cpp
3472

Let's always call signature help and code completion here to be consistent with other cases.
We don't want to ever block completion, even in obscure cases when it's called twice.

lib/Sema/SemaDeclCXX.cpp
3784

NIT: invert condition to reduce nesting and simplify the code, i.e.

if (SS.getScopeRep() || TemplateTypeTy)
  return nullptr;
// rest of the code
3785

This comment does not look useful without a context, maybe move it to the previous call site in sema?

ilya-biryukov added inline comments.Sep 11 2018, 5:22 AM
include/clang/Sema/Sema.h
4571

s/decleration/declaration.

Maybe even remove the comment? the name is self-descriptive

kadircet updated this revision to Diff 164864.Sep 11 2018, 6:52 AM
kadircet marked 3 inline comments as done.
  • Resolve discussions.
kadircet marked an inline comment as done.Sep 11 2018, 6:53 AM
kadircet updated this revision to Diff 164866.Sep 11 2018, 6:59 AM
  • Update tests.
ilya-biryukov accepted this revision.Sep 11 2018, 7:38 AM

LGTM. Thanks for the fix!

This revision is now accepted and ready to land.Sep 11 2018, 7:38 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.