Page MenuHomePhabricator

[clang] Improve ctor initializer completions.
ClosedPublic

Authored by kadircet on Oct 24 2018, 9:31 AM.

Details

Summary

Instead of providing generic "args" for member and base class
initializers, tries to fetch relevant constructors and show their signatures.

Diff Detail

Repository
rC Clang

Event Timeline

kadircet created this revision.Oct 24 2018, 9:31 AM

Great idea, this will definitely improve the UX of completion!

NIT: a typo in the change description: 'bas' should be 'base'.
Pedantic NIT (sorry!): in the change description, we should probably use 'initializers' instead of 'initializations'

lib/Sema/SemaCodeComplete.cpp
812

Maybe add the ctor results directly to the result list without creating an intermediate vector?
See other comments about passing the field name to this function instead of replacing it in the resulting CCS afterwards.

5080

NIT: maybe pass the Builder explicitly? Even a local function that changes the state and without explicit data-flow (i.e. without accepting the thing it changes as a parameter) is a bit confusing?

5088

Why special-case the template specializations?
Are we trying to provide results for dependent types here?

5090

It seems this function is sometimes responsible for building the completion string and sometimes it's the responsibility of the calling function.
Could we move this responsibility to one of the places (probably the callers?)

5195

NIT: use early exits to simplify code.
I.e.

// NOTE: Moved from the end of the loop.
SawLastInitializer = false;

const CXXRecordDecl *RD = Field->getType()->getAsCXXRecordDecl();
if (!RD) {
  CreatePrimitiveConstructor(Builder.getAllocator().CopyString(FieldName));
  AddField(Field);
  continue;
}
...
5196

NIT: the ConstResults name is a bit confusing, because "const" has a another meaning in C++. Maybe rename to CtorResults and ConstructorResults?

5204

Could we pass the field name all the way down to the function that creates the original CodeCompletionString? (Instead of replacing the ctor name with a field name at the end)
This would mean less copies/allocations going around and would also be simpler to reason about (i.e. no "incomplete" code completion strings would be passed around).

5221

Maybe show a type of the field for non-class types? They can typically be initialized from a single value of that type (with some exceptions, but showing the type shouldn't be confusing).
E.g.

struct X {
  X() : ^ {} // could complete 'a({$1:int})`
  int a;
  int b;
};
kadircet edited the summary of this revision. (Show Details)Oct 25 2018, 2:35 AM
kadircet updated this revision to Diff 171077.Oct 25 2018, 6:07 AM
kadircet marked 7 inline comments as done.
  • Address comments.
lib/Sema/SemaCodeComplete.cpp
5088

Sorry, my bad forgot to add a comment. It is rather for backward compatibility. Since previous version was adding results directly without consulting results, it also added template specializations, whereas current version can't add them due to the filtering in resultbuilder(https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaCodeComplete.cpp#L543). So, special casing in here to make sure we still provide those results, but can be deleted if seems unimportant.

Not adding a comment until we decide on keeping/deleting the special case.

kadircet added inline comments.Oct 25 2018, 6:09 AM
lib/Sema/SemaCodeComplete.cpp
5088

without consulting results -> without consulting "ResultBuilder"

ilya-biryukov added inline comments.Oct 25 2018, 6:26 AM
lib/Sema/SemaCodeComplete.cpp
5088

Maybe call AddResult instead of MaybeAddResult (it does not do the checking)?
I can hardly see why we might want to filter out any of the results.

Also, maybe pass a decl the particular constructor, rather than the CXXRecordDecl to the ResultBuilder?

kadircet updated this revision to Diff 171099.Oct 25 2018, 7:54 AM
kadircet marked an inline comment as done.
  • Use addConstructorResult.
kadircet updated this revision to Diff 171259.Oct 26 2018, 1:33 AM
  • Add a generic constructor if found none.
  • format && add comments.
  • Add generic constructors to tests.
ilya-biryukov added inline comments.Oct 29 2018, 2:12 AM
include/clang/Sema/CodeCompleteConsumer.h
767 ↗(On Diff #171259)

NIT: does "identifiers name" refer to a field name? Maybe use "field name" directly to avoid possibly confusion.

lib/Sema/SemaCodeComplete.cpp
2982

Maybe directly set the name here whenever PreferredTypedName is set and avoid changing the AddTypedNameChunk function?
This would keep the AddTypedNameChunk simpler and PreferredTypedName would simply become an overriden name that the code completion string should use instead of the declaration name.
I.e.

if (PreferredTypedName)
  Result.AddTypedTextChunk(Result.getAllocator().CopyString(PreferredTypedName));
else
  AddTypedNameChunk(Ctx, Policy, ...);
5198

Should we also have fallbacks for bases in addition to fields? They may also have non-class types.
Could we also add a test?
Should pop up in completions of dependent bases, e.g.

template <class T>
struct Y : T {};

template <class T>
struct X : std::vector<T> {}; // referring to an explicit template, however still dependent.
5199

Maybe add a comment that this is a fallback?

test/CodeCompletion/ordinary-name-cxx11.cpp
65 ↗(On Diff #171259)

This completion is outside constructor initializers.
Why behavior change for these completions too?

kadircet updated this revision to Diff 171516.Oct 29 2018, 9:07 AM
kadircet marked 5 inline comments as done.
  • Address comments & offline discussions.
ZaMaZaN4iK added inline comments.
lib/Sema/SemaCodeComplete.cpp
5092

Is it good idea to capture ALL by reference? Probably will be better to capture only required things by reference

kadircet added inline comments.Oct 30 2018, 2:02 AM
lib/Sema/SemaCodeComplete.cpp
5092

That's the case within the rest of the code, so I did that to be consistent, and apart from that I don't think this brings any disadvantages.

ilya-biryukov added inline comments.Oct 30 2018, 3:07 AM
lib/Sema/SemaCodeComplete.cpp
811

We don't seem to need this fwd-decl anymore. Remove it?

819

There's a function that does the same thing and also does some extra lifting to make sure implicit members are properly generated - Sema::LookupConstructors. Maybe use it instead?

829

Remove this code instead of commenting it out?

842

Maybe use range-baseed for loop?

5080

NIT: naming, use UpperCamelCase

5092

TypedName and TypeName are too similar. Maybe pick names that are easier to distinguish?
Name and Type seem to be a good fit

5093

Maybe use StringRef?

5108

CodeCompletionResult R looks redundant here? Do we really need it?

5123

The code past that point is the same between AddBase and AddField. Maybe move it into AddCtorsWithName?

kadircet updated this revision to Diff 171703.Oct 30 2018, 8:07 AM
kadircet marked 8 inline comments as done.
  • Address comments.
lib/Sema/SemaCodeComplete.cpp
819

As talked offline we don't want this behavior.

829

Ah, sorry for the mess :(

5093

Don't think this looks any better, since we need to use .data() at chunk additions than.

Even if we try to push allocations all the way down until we add those chunks, we still get bad looking code since types are returned as std::string and we need to make a copy in the stack to be able to pass it as StringRef.

Mostly NITs

lib/Sema/SemaCodeComplete.cpp
811

Maybe make the name shorter?
E.g. getConstructors or lookupConstructors?

843

Maybe inline Ctors?
I.e.

for (auto Ctor : getConstructorResults(SemaRef.Context, Record))
5081

Builder is not used outside GenerateCCS and AddDefaultCtorInit, maybe move its declaration into those lamdbas and remove from the enclosing function?
Would mean less mutable state to care about and the function seems big enough for that to matter.

5083

NIT: use auto

5085

NIT: use auto

5093

char* and StringRef have equivalent lifetime properties, so the set of cases you need to do extra copies is equivalent. Am I missing something?
But yeah, since AddChunk methods accept a char*, StringRef is not suitable because it's not guaranteed to be null-terminated. LG

test/CodeCompletion/ctor-initializer.cpp
44

Maybe use a single check CHECK-CC5-NOT: COMPLETION: Pattern : Base? (i.e. not mentioning the arguments)

68

Why do we want to change this test?

test/Index/complete-ctor-inits.cpp
33

Reading through the code, it seems MemberRef was the correct CursorKind here (the docs mention that MemberRef is a reference in non-expression context and ctor-initializer seems to be one of those).
Maybe keep it a MemberRef, since it looks simple enough.

Having CXXConstrutor instead of NotImpleneted is clearly a win, though :-)

kadircet updated this revision to Diff 171899.Oct 31 2018, 5:55 AM
kadircet marked 8 inline comments as done.
  • Address comments.
kadircet added inline comments.Oct 31 2018, 6:08 AM
lib/Sema/SemaCodeComplete.cpp
5093

Yes, but I store those strings inside allocator and pass pointers of those stored ones. If I were to defer the allocation, I would need to keep copies of the strings myself until I call allocate on them.

test/CodeCompletion/ctor-initializer.cpp
68

This was for something else I've been testing, nvm.

The change below is however, to get rid of the compile error.

ilya-biryukov added inline comments.Oct 31 2018, 7:59 AM
lib/Sema/SemaCodeComplete.cpp
176

AddConstructorResults is not defined and not used. Remove it?

kadircet updated this revision to Diff 171926.Oct 31 2018, 8:19 AM
  • Revert back declaration.
kadircet marked an inline comment as done.Oct 31 2018, 8:20 AM
ilya-biryukov accepted this revision.Oct 31 2018, 8:35 AM

LGTM. Thanks for the change!

This revision is now accepted and ready to land.Oct 31 2018, 8:35 AM
This revision was automatically updated to reflect the committed changes.