<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jul 6, 2011, at 2:16 PM, Connor Wakamo wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>I've made a few changes in response to your comments (noted below) and have a couple more questions.<br><br><blockquote type="cite">Very cool. Some comments on your patch:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ /**<br></blockquote><blockquote type="cite">+ * \brief The context for completions is unknown or unexposed.<br></blockquote><blockquote type="cite">+ */<br></blockquote><blockquote type="cite">+ CXCompletionContext_Unknown = 0,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">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?<br></blockquote><br>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?)<br></div></blockquote><div><br></div>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.</div><div><br></div><div>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).</div><div><br></div><div><br><blockquote type="cite"><div><blockquote type="cite">+ /**<br></blockquote><blockquote type="cite">+ * \brief Completions for fields of the member being accessed should be<br></blockquote><blockquote type="cite">+ * included in the results.<br></blockquote><blockquote type="cite">+ */<br></blockquote><blockquote type="cite">+ CXCompletionContext_MemberAccess = 1 << 5,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">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.<br></blockquote><br>Would it be fine to just add something like CXCompletionContext_ObjCPropertyAccess, or should I split it further (to something like ObjCPropertyAccess, ArrowMemberAccess, DotMemberAccess)?<br></div></blockquote><div><br></div><div>I think it makes sense to provide ArrowMemberAccess and DotMemberAccess, since we also care about the answer to this question with C++ classes:</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>template<typename T></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>class shared_ptr {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span> T *ptr;</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>public:</div><div><span class="Apple-tab-span" style="white-space:pre"> </span> T* operator->();</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>};</div><br><blockquote type="cite"><div><blockquote type="cite">+ /**<br></blockquote><blockquote type="cite">+ * \brief Completions for Objective-C instance methods should be included<br></blockquote><blockquote type="cite">+ * in the results.<br></blockquote><blockquote type="cite">+ */<br></blockquote><blockquote type="cite">+ CXCompletionContext_ObjCInstanceMethod = 1 << 14,<br></blockquote><blockquote type="cite">+ /**<br></blockquote><blockquote type="cite">+ * \brief Completions for Objective-C class methods should be included in<br></blockquote><blockquote type="cite">+ * the results.<br></blockquote><blockquote type="cite">+ */<br></blockquote><blockquote type="cite">+ CXCompletionContext_ObjCClassMethod = 1 << 15,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">These seem to be tied up to CCC_ObjCInstanceMessage/CCC_ObjCClassMessage, which are for message sends, e.g.,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span>[x initWithFoo:foo andBar:bar]<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">but the nacd llvmmes and comments seem to imply that we're talking about, e.g.,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span>- (id)initWithFoo:(Foo*)foo andBar:(Bar*)bar]<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">?<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">If it's message sends that we're targeting, we're going to need to expose the selector pieces that have already been <br></blockquote><br>Yes, this is targeting message sends; I've renamed the values and updated the comments to be clearer.<br></div></blockquote><div><br></div><div>Looks good, thanks.</div><br><blockquote type="cite"><div>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?)<font class="Apple-style-span" color="#00721d"><br></font></div></blockquote><div><br></div><div>Yes. For completions of message sends to be useful to the client, we'll need to expose this information.</div><br><blockquote type="cite"><div><blockquote type="cite">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.,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span>struct A { };<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span>A *ptr;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">(We don't want the client to have to think about that)<br></blockquote><br>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?<br></div></blockquote><div><br></div>Sure. Why not just call it CXCompletionContext_ClassTag, to match the others?</div><div><br></div><div><blockquote type="cite"><div><blockquote type="cite">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 "::".<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">CCC_PotentiallyQualifiedName seems like it makes onto the "nested name specifiers" bit I mentioned above.<br></blockquote><br>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.<br></div></blockquote><div><br></div><div>Hrm. I think we don't want NestedNameSpecifier here:</div><div><br></div><div><div>+ case CodeCompletionContext::CCC_Namespace: {</div><div>+ contexts = CXCompletionContext_Namespace |</div><div>+ CXCompletionContext_NestedNameSpecifier;</div><div>+ break;</div><div>+ }</div><div><br></div><div>Otherwise, looks good!</div></div><br><blockquote type="cite"><div><blockquote type="cite">+ case CodeCompletionContext::CCC_MacroName:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This maps to CXCompletionContext_MacroName?<br></blockquote><br>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.<br></div></blockquote><div><br></div><div>Ahh, okay. I understand.</div><br><blockquote type="cite"><div><blockquote type="cite">CCC_TypeQualifiers is one of those cases where we probably want to return a completely empty set.<br></blockquote><br>See above for my note on CXCompletionContext_Unknown.<br></div></blockquote><div><br></div><div>Same here :)</div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><br></body></html>