Instead of providing generic "args" for member and base class
initializers, tries to fetch relevant constructors and show their signatures.
Details
Diff Detail
- Repository
- rC Clang
- Build Status
Buildable 24382 Build 24381: arc lint + arc unit
Event Timeline
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 | ||
---|---|---|
814 | Maybe add the ctor results directly to the result list without creating an intermediate vector? | |
5082 | 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? | |
5090 | Why special-case the template specializations? | |
5092 | It seems this function is sometimes responsible for building the completion string and sometimes it's the responsibility of the calling function. | |
5197 | NIT: use early exits to simplify code. // 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; } ... | |
5198 | NIT: the ConstResults name is a bit confusing, because "const" has a another meaning in C++. Maybe rename to CtorResults and ConstructorResults? | |
5206 | 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) | |
5223 | 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). struct X { X() : ^ {} // could complete 'a({$1:int})` int a; int b; }; |
- Address comments.
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
5090 | 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. |
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
5090 | without consulting results -> without consulting "ResultBuilder" |
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
5090 | Maybe call AddResult instead of MaybeAddResult (it does not do the checking)? Also, maybe pass a decl the particular constructor, rather than the CXXRecordDecl to the ResultBuilder? |
- Add a generic constructor if found none.
- format && add comments.
- Add generic constructors to tests.
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 | ||
2984 | Maybe directly set the name here whenever PreferredTypedName is set and avoid changing the AddTypedNameChunk function? if (PreferredTypedName) Result.AddTypedTextChunk(Result.getAllocator().CopyString(PreferredTypedName)); else AddTypedNameChunk(Ctx, Policy, ...); | |
5200 | Should we also have fallbacks for bases in addition to fields? They may also have non-class types. template <class T> struct Y : T {}; template <class T> struct X : std::vector<T> {}; // referring to an explicit template, however still dependent. | |
5201 | 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. |
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
5094 | Is it good idea to capture ALL by reference? Probably will be better to capture only required things by reference |
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
5094 | 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. |
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
813 | We don't seem to need this fwd-decl anymore. Remove it? | |
821 | 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? | |
831 | Remove this code instead of commenting it out? | |
844 | Maybe use range-baseed for loop? | |
5082 | NIT: naming, use UpperCamelCase | |
5094 | TypedName and TypeName are too similar. Maybe pick names that are easier to distinguish? | |
5095 | Maybe use StringRef? | |
5110 | CodeCompletionResult R looks redundant here? Do we really need it? | |
5125 | The code past that point is the same between AddBase and AddField. Maybe move it into AddCtorsWithName? |
- Address comments.
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
821 | As talked offline we don't want this behavior. | |
831 | Ah, sorry for the mess :( | |
5095 | 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 | ||
---|---|---|
813 | Maybe make the name shorter? | |
845 | Maybe inline Ctors? for (auto Ctor : getConstructorResults(SemaRef.Context, Record)) | |
5083 | Builder is not used outside GenerateCCS and AddDefaultCtorInit, maybe move its declaration into those lamdbas and remove from the enclosing function? | |
5085 | NIT: use auto | |
5087 | NIT: use auto | |
5095 | char* and StringRef have equivalent lifetime properties, so the set of cases you need to do extra copies is equivalent. Am I missing something? | |
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). Having CXXConstrutor instead of NotImpleneted is clearly a win, though :-) |
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
5095 | 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. |
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
176 | AddConstructorResults is not defined and not used. Remove it? |
AddConstructorResults is not defined and not used. Remove it?