[PATCH] D96058: [CodeComplete] Guess type for designated initializers

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clang/lib/Parse/ParseInit.cpp:163
+    DesignatorCompletionInfo DesignatorCompletion) {
+  if (!getPreprocessor().isCodeCompletionEnabled())
+    DesignatorCompletion.PreferredBaseType = QualType(); // skip field lookup
----------------
kadircet wrote:
> it might be nice to make the whole preferredtypebuilder a no-op on construction when codecompletion is disabled. but that can be an adventure for another day.
Agree, this felt awkward. There's already a laziness mechanism (the function-arg stuff) but it doesn't quite fit for this case.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4803
+      if (CTSD->getTemplateSpecializationKind() == TSK_Undeclared)
+        RD = CTSD->getSpecializedTemplate()->getTemplatedDecl();
+    }
----------------
kadircet wrote:
> i think this means we are going to offer completion for uninstantiated specializations using the primary template now (through `Sema::CodeCompleteMemberReferenceExpr`), which i am not sure is possible, but should be an improvement in either case.
> 
> but not having any tests (especially failing) makes me a little anxious.
I think this case is vacuous for CodeCompleteMemberExpr, like you say.

Before calling CodeCompleteMemberExpr, we call ActOnStartCXXMemberReference. If it's a non-dependent record type, then it calls RequireCompleteType, which will try to instantiate the template. If this fails, then we bail out and never invoke code completion.

So I think there's no change in behavior for member completion, and thus can't produce a test for it. The reason for moving the fallback case into this function is that instead of 2 callsites where 1 needed the fallback, we now have 3 callsites where 2 need it. So it seems less awkward to factor this way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96058/new/

https://reviews.llvm.org/D96058