[cfe-commits] [PATCH] New libclang API function, clang_codeCompleteGetContexts

[Douglas Gregor]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110706/e6fca35c/attachment.html>