[PATCH] D62238: [CodeComplete] Complete a lambda when preferred type is a function

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4135
+        !PotentialTemplateName.getAsIdentifierInfo()->getName().contains(
+            "function"))
+      return nullptr;
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > This looks cheesy, do we really want to perform this operation only for templates with `"function"` in their names?
> > > 
> > > As discussed offline maybe perform this for any template with a single(non-defaulted?) argument, or maybe even better perform a type-check as suggested by you. But I believe there would be too many false positives with the current state.
> > WDYT about `"function"` + only template with a single arg?
> > FWIW, I think function in template arguments are very rare, so I'm more optimistic about false-positives even in the current form.
> > 
> > We have an option of making it super-restrictive and only firing on whitelisted things like `std::function`, `boost::function`, etc.
> > But we don't have a mechanism to configure those from the outside, so that will probably turn out too restrictive.
> I don't think adding "function" as a restriction is helping much on reducing false positives. I would rather get rid of that check to increase usability.
> 
> The real problem is rather checking constructors to make sure there is at least one for which we can provide a callable with the signature we found. Anything else just seems cheesy and might result in people adding more and more cases over time.
> 
> It is up to you, I am OK if you choose to keep this restriction as well.
Removed the restriction on the function way.
I don't think there's an easy way to check for conversions, though, I hope the false-positive rate would super-small.
If not, I'll find a way to check conversions or whitelist only known function names.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4125
+    PotentialFunctionArg = &Specialization->getArg(0);
+  } else if (auto *Class = dyn_cast_or_null<ClassTemplateSpecializationDecl>(
+                 T->getAsCXXRecordDecl())) {
----------------
kadircet wrote:
> I thought you decided to get rid of this branch after the offline discussions, sorry for not mentioning in earlier revision.
Sorry, my bad, forgot about it.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4142
+  // Handle other cases.
+  if (T->isPointerType())
+    T = T->getPointeeType();
----------------
kadircet wrote:
> what about referencetype?
> 
> ```
> void test();
> void (&y)() = test;
> ```
Lambdas are r-values, so suggesting them for l-value references would lead to an error.
R-value references to functions are so rare that I feel handling those is not worth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62238