[PATCH] D70445: [clangd] Show lambda signature for lambda autocompletions

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2474
 
+TEST(CompletionTest, Lambda) {
+  clangd::CodeCompleteOptions Opts = {};
----------------
testing this in clangd is nice to have, but the canonical test for stuff in sema code completion should be a lit test under clang/test/CodeCompletion IIRC


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2476
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
----------------
specializing opts shouldn't be needed for this test


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2486
+  EXPECT_EQ(Results.Completions.size(), 1u);
+  const auto A = Results.Completions.front();
+  EXPECT_EQ(A.Name, "Lambda");
----------------
auto&


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3330
 
+namespace {
+const CXXMethodDecl *extractLambdaCallOperator(const NamedDecl *ND) {
----------------
llvm style is to make the function static instead (we don't follow this particlularly closely in clangd, but we should here)


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3331
+namespace {
+const CXXMethodDecl *extractLambdaCallOperator(const NamedDecl *ND) {
+  const auto *VD = dyn_cast<VarDecl>(ND);
----------------
in view of the functor (std::function etc) case, should this be extractFunctorCallOperator()?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3335
+    return nullptr;
+  const auto *LambdaType = VD->getType().getTypePtr();
+  if (const auto *SugaredType = dyn_cast<AutoType>(LambdaType))
----------------
VD->getType()->getAsCXXRecordDecl() should be all you need.
getAs() and friends desugar as needed.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3342
+  const auto *RecordDecl = LambdaType->getAsCXXRecordDecl();
+  // FIXME: Don't ignore generic lambdas.
+  return (RecordDecl && RecordDecl->isLambda() &&
----------------
can we fix that instead of leaving a FIXME? should only be a couple of lines I think


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3342
+  const auto *RecordDecl = LambdaType->getAsCXXRecordDecl();
+  // FIXME: Don't ignore generic lambdas.
+  return (RecordDecl && RecordDecl->isLambda() &&
----------------
sammccall wrote:
> can we fix that instead of leaving a FIXME? should only be a couple of lines I think
also FIXME: support other functor types like std::function?

looking at the implementation of getLambdaCallOperatorHelper it looks pretty easy


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3374
 
-  AddResultTypeChunk(Ctx, Policy, ND, CCContext.getBaseType(), Result);
-
-  if (const auto *Function = dyn_cast<FunctionDecl>(ND)) {
-    AddQualifierToCompletionString(Result, Qualifier, QualifierIsInformative,
-                                   Ctx, Policy);
+  auto handleFunctionlike = [&](const FunctionDecl *Function,
+                                bool AddQualifier = true) {
----------------
can you give this a clearer name?
Like AddFunctionTypeAndResult?

I also think making it void would be clearer, rather than mixing adding/building.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3378
+    if (AddQualifier)
+      AddQualifierToCompletionString(Result, Qualifier, QualifierIsInformative,
+                                     Ctx, Policy);
----------------
why do you want to avoid adding the qualifier in lambda case?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3396
+
+  AddResultTypeChunk(Ctx, Policy, ND, CCContext.getBaseType(), Result);
+
----------------
above the return statement to below - why?


Repository:
  rC Clang

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

https://reviews.llvm.org/D70445