[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this does look better.

I agree with Nathan's comments and will leave final stamp to him.



================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:144
+
+  // This variable would be discarded directly at the end of this function. We
+  // store part of the chunks of snippets here if DropFunctionArguments is
----------------
I find this comment very unclear, I think because it starts with implementation details and works its way up to general principles.

I think `Buffer that snippet chunks are written to once we've decided to discard the snippet due to DropFunctionArguments` or so would be enough.

However I'd rather drop this variable entirely if possible, the data flow is confusing. See below.


================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:148
+  // Signature for the function would be preserved.
+  // It is preferable if we don't produce the arguments at the clang site. But
+  // that would also lose the signatures, which could sometimes help users to
----------------
For the reason you give here, it's actually *not* preferable. So I'd suggest leaving out this comment.


================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:258
     case CodeCompletionString::CK_RightParen:
+      if (DropFunctionArguments &&
+          ResultKind == CodeCompletionResult::RK_Declaration)
----------------
nridge wrote:
> It looks like we are assuming two things:
> 
>  1. Any LeftParen in an RK_Declaration starts a function argument list
>  2. Everything that comes after the function argument list can be discarded
> 
> I think these assumptions are fine to make, but let's be explicit about them in a comment
Agree, but also I think the code could reflect this more directly:
 - this should trigger only for CK_LeftParen, not CK_RightParen

Rather than redirecting output to a different string by reassigning the param, I think it would be a bit more direct to have

```
optional<unsigned> TruncateSnippetAt;
...
case CK_LeftBracket:
  if (DropFunctionArguments && ... && !TruncateSnippetAt)
    TruncateSnippetAt = Snippet->size();
...
if (TruncateSnippetAt)
  Snippet->resize(*TruncateSnippetAt);
}
```

(though still not totally clear)


================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.h:45
 ///
+/// \p DropFunctionArguments indicates that the function call arguments should
+/// be omitted from the \p Snippet. If enabled, the \p Snippet will only contain
----------------
Prefer positive sense for bool params (`IncludeFunctionArguments`) to avoid double-negative confusion


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1387
 
-  // When completing a non-static member function (and not via
-  // dot/arrow member access) and we're not inside that class' scope,
-  // it can't be a call.
+  // Decide whether or not a non-static member function can be a call.
   if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol) {
----------------
This is confusing: the `CCC_Symbol` test is part of the specific heuristics being used (it's the "not via dot/arrow member access" part, right?) but you've moved the comment explaining what it does.

Also this function is just getting too long, and we're inlining more complicated control flow here.
Can we extract a function?

```
const auto *Method = ...;
if (Method & !Method->isStatic()) {
  R.FunctionCanBeCall = canMethodBeCalled(...);
}    
```


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1417
+
+      // If the member access "." or "->" is followed by a qualified Id and the
+      // object type is derived from or the same as that of the Id, then
----------------
This description is hard for me to follow, and it's hard to tell if this is independent of the previous check, or an exception to it.

An example would be clearer. (I think instead rather than in addition):
`Exception: foo->FooBase::bar() *is* a call`.




================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1420
+      // the candidate functions should be perceived as calls.
+      if (const CXXRecordDecl *MaybeDerived = nullptr;
+          !BaseType.isNull() &&
----------------
`if (const CXXRecordDecl *BaseDecl = BaseType ? BaseType-> getAsCXXRecordDecl() : nullptr)`?
avoiding the reassignment inside a boolean expression


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156605