On Jul 6, 2011, at 2:16 PM, Connor Wakamo wrote:
> I've made a few changes in response to your comments (noted below) and have a
> couple more questions.
>
>> Very cool. Some comments on your patch:
>>
>> + /**
>> + * \brief The context for completions is unknown or unexposed.
>> + */
>> + CXCompletionContext_Unknown = 0,
>>
>> How should the client interpret this bit? Is it the same as setting all of
>> the bits in the result set, and if so, do we need it?
>
> Internally, CXCompletionContext_Unknown means Clang doesn't have the right
> information to provide better context or, as you noted below with
> CCC_TypeQualifiers, it means that Clang has already returned everything
> that's acceptable. A client should interpret this as "only Clang results are
> acceptable," not "everything is acceptable." I've updated the comment to
> make this clearer. (Should this be renamed to something else, or split into
> separate Unknown and Unexposed contexts?)
If only Clang results are acceptable, I think the right way to handle this is
to have no bits set. A client can always test for this and short-circuit its
own search.
If Clang doesn't know what context it's in, then it should go ahead and set all
of the bits, and the client will add everything it knows about. (This happens
when parsing got confused).
>> + /**
>> + * \brief Completions for fields of the member being accessed should be
>> + * included in the results.
>> + */
>> + CXCompletionContext_MemberAccess = 1 << 5,
>>
>> This doesn't seem specific enough. For example, it doesn't allow one to tell
>> the difference between -> and . on an Objective-C object, so the client
>> wouldn't know whether to produce ivars or properties.
>
> Would it be fine to just add something like
> CXCompletionContext_ObjCPropertyAccess, or should I split it further (to
> something like ObjCPropertyAccess, ArrowMemberAccess, DotMemberAccess)?
I think it makes sense to provide ArrowMemberAccess and DotMemberAccess, since
we also care about the answer to this question with C++ classes:
template<typename T>
class shared_ptr {
T *ptr;
public:
T* operator->();
};
>> + /**
>> + * \brief Completions for Objective-C instance methods should be included
>> + * in the results.
>> + */
>> + CXCompletionContext_ObjCInstanceMethod = 1 << 14,
>> + /**
>> + * \brief Completions for Objective-C class methods should be included in
>> + * the results.
>> + */
>> + CXCompletionContext_ObjCClassMethod = 1 << 15,
>>
>> These seem to be tied up to CCC_ObjCInstanceMessage/CCC_ObjCClassMessage,
>> which are for message sends, e.g.,
>>
>> [x initWithFoo:foo andBar:bar]
>>
>> but the nacd llvmmes and comments seem to imply that we're talking about,
>> e.g.,
>>
>> - (id)initWithFoo:(Foo*)foo andBar:(Bar*)bar]
>>
>> ?
>>
>> If it's message sends that we're targeting, we're going to need to expose
>> the selector pieces that have already been
>
> Yes, this is targeting message sends; I've renamed the values and updated the
> comments to be clearer.
Looks good, thanks.
> As for your second point, do you mean that clang should expose the
> information about the selector pieces that have already been typed? (So, for
> instance, if I asked for code completion results right before "andBar:" in
> the above example, would there need to be a separate function to return
> "initWithFoo:", possibly along with type information for foo?)
Yes. For completions of message sends to be useful to the client, we'll need to
expose this information.
>> Wherever "contexts" includes CXCompletionContext_AnyType, it seems that we
>> also want to include
>> CXCompletionContext_EnumTag/CXCompletionContext_UnionTag/CXCompletionContext_StructTag
>> when we're in C++ mode, for, e.g.,
>>
>> struct A { };
>> A *ptr;
>>
>> (We don't want the client to have to think about that)
>
> I've made that change as well; additionally, I added
> CXCompletionContext_ClassName (along with EnumTag/UnionTag/StructTag) when
> we're in C++ mode. Is that correct?
Sure. Why not just call it CXCompletionContext_ClassTag, to match the others?
>> Similarly, we can have a qualified name just about everywhere in C++, which
>> makes things a bit interesting... perhaps we want a bit for "nested name
>> specifiers", like ASTUnit does, and the client can provide completions for
>> namespaces/classes/etc. followed by "::".
>>
>> CCC_PotentiallyQualifiedName seems like it makes onto the "nested name
>> specifiers" bit I mentioned above.
>
> I've added a bit for CXCompletionContext_NestedNameSpecifier, and have put it
> where it should go. Let me know if I've missed any contexts.
Hrm. I think we don't want NestedNameSpecifier here:
+ case CodeCompletionContext::CCC_Namespace: {
+ contexts = CXCompletionContext_Namespace |
+ CXCompletionContext_NestedNameSpecifier;
+ break;
+ }
Otherwise, looks good!
>> + case CodeCompletionContext::CCC_MacroName:
>>
>> This maps to CXCompletionContext_MacroName?
>
> Actually, it doesn't. It might be a case where the value names are a bit
> confusing, and let me know if I should change it, but CCC_MacroName means
> that code completion is occurring where a macro is being defined and it needs
> a name. (At least, that's what the comment says in
> Sema/CodeCompleteConsumer.h.) I've mapped it to CXCompletionContext_Unknown,
> as I don't think it would be possible to suggest completions for a new name.
> (Correct me if I'm wrong on that.) CCC_MacroNameUse, on the other hand, does
> map to CXCompletionContext_MacroName, as that context is asking for one that
> already exists.
Ahh, okay. I understand.
>> CCC_TypeQualifiers is one of those cases where we probably want to return a
>> completely empty set.
>
> See above for my note on CXCompletionContext_Unknown.
Same here :)
- Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
