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

[Connor Wakamo]

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?)

> +  /**
> +   * \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)?

> +  /**
> +   * \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.

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?)

> +  /// \brief Base type for member access (CCC_MemberAccess) completions.
> +  clang::QualType BaseType;
> +  
> +  /// \brief The preferred type for the current completion context.
> +  clang::QualType PreferredType;
> 
> Hrm, interesting. There's a problem here, which is that the code completion results can actually outline the CXTranslationUnit that the QualType points into, so if we're going to save this information, we'll have to do it in some way that doesn't reference back into the ASTContext. Since this information isn't exposed in the API now, I suggest dropping it.

I've removed these two QualTypes, as well as an #include that I had added.

> 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?

> 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.

> +    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.

> 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.

> 
> // CHECK-OVERLOAD: NotImplemented:{ResultType int &}{Text overloaded}{LeftParen (}{Text Z z}{Comma , }{CurrentParameter int second}{RightParen )}
> // CHECK-OVERLOAD: NotImplemented:{ResultType float &}{Text overloaded}{LeftParen (}{Text int i}{Comma , }{CurrentParameter long second}{RightParen )}
> // CHECK-OVERLOAD: NotImplemented:{ResultType double &}{Text overloaded}{LeftParen (}{Text float f}{Comma , }{CurrentParameter int second}{RightParen )}
> +// CHECK-OVERLOAD: Completion contexts:
> +// CHECK-OVERLOAD: Any type
> +// CHECK-OVERLOAD: Any value
> +// CHECK-OVERLOAD: Objective-C interface
> 
> You could use CHECK-OVERLOAD-NEXT for the last three of these lines, to make sure that the test fails if any additional contexts sneak in. 

I've made this change as well to all of the tests that I updated.

Also, thanks Tobias for catching my typos.

Thanks,
Connor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang_codeCompleteGetContexts_update.patch
Type: application/octet-stream
Size: 28115 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110706/9bd3ba83/attachment.obj>