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
clang_codeCompleteGetContexts_update.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
