zyounan added a comment.
Thanks for Sam and Nathan's thorough review! I've updated, and please take
another look.
(Apologize for churning on weekends, but I don't have chunk time during the
week.)
================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:258
case CodeCompletionString::CK_RightParen:
+ if (DropFunctionArguments &&
+ ResultKind == CodeCompletionResult::RK_Declaration)
----------------
sammccall wrote:
> 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)
> It looks like we are assuming two things
Honestly, I don't quite prefer this assumption. However, we have lost some of
the semantics e.g., the structure of a call expression within this function,
and unfortunately it's not trivial to pass these out from clang. :-(
> (though still not totally clear)
Yeah. I imagine a clearer way would be separating the calculation for
`Signature` and `Snippet`, and we could bail out early for `Snippet`. But I'm
afraid that making so many adjustments to `getSignature` for such a small
feature is inappropriate.
Assuming the left parenthesis is the beginning of a call might be sufficient
for the time being, and let's leave the refactoring to subsequent patches.
================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:580
auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
EXPECT_THAT(Results.Completions,
Contains(AllOf(named("method"), snippetSuffix(""))));
----------------
nridge wrote:
> Since you're updating this test anyways, could you add `signature()` matchers
> for the non-template cases as well please?
Of course!
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:345
void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding,
- bool InBaseClass);
+ bool InBaseClass, QualType BaseType);
----------------
nridge wrote:
> The word "base" is a bit overloaded in this signature; in the parameter
> `InBaseClass` it refers to inheritance, but in `BaseType` it refers to the
> base expression of a `MemberExpr`.
>
> Maybe we can name the new parameter `BaseExprType` to avoid confusion?
Makes sense to me. Thanks for the correction.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1285
Result.ShadowDecl = Using;
AddResult(Result, CurContext, Hiding);
return;
----------------
nridge wrote:
> Should we propagate `BaseType` into this recursive call?
Yes. And thanks for spotting this. I added another test case reflecting it.
================
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)
{
----------------
sammccall wrote:
> 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(...);
> }
> ```
> it's the "not via dot/arrow member access" part
(Sorry for being unaware of the historical context). But I think `CCC_Symbol`
should mean "we're completing a name such as a function or type name" per its
comment. The heuristic for dot/arrow member access actually lies on the next
line, i.e., if the completing decl is a CXXMethodDecl.
> Can we extract a function?
Sure.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1425
+ R.FunctionCanBeCall =
+ MaybeDerived == MaybeBase ||
MaybeDerived->isDerivedFrom(MaybeBase);
+ }
----------------
nridge wrote:
> Is there any situation where `MaybeDerived == MaybeBase ||
> MaybeDerived->isDerivedFrom(MaybeBase)` is false?
>
> I am wondering if we can simplify this to:
>
> ```
> if (!BaseType.isNull()) {
> R.FunctionCanBeCall = true;
> }
> ```
I think this could prevent us from completing ill-formed expressions like:
```
struct B {
void foo();
};
struct D : B {
void bar();
};
struct S {
void method();
};
void baz() {
D().S::^
}
```
Currently, we have the candidate `method`, and completing it with parentheses
is definitely wrong. (Although we shouldn't be providing `method` as well.)
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3577
// containing all of the arguments up to the first deducible argument.
+ // Or, if this isn't a call, emit all the template arguments
+ // to disambiguate the (potential) overloads.
----------------
nridge wrote:
> 1. If this is not a call, we can avoid running the
> `Sema::MarkDeducedTemplateParameters` operation altogether.
>
> 2. A future improvement we could consider: if this is not a call, try to
> detect cases where the template parameters can be deduced from the
> surrounding context (["Deducing template arguments taking the address of a
> function template "](https://eel.is/c++draft/temp.deduct.funcaddr)). Maybe
> add a FIXME for this?
> avoid running the Sema::MarkDeducedTemplateParameters operation altogether
I think doing so could cause too many adjustments to the flow, and I'm afraid
that the `Sema::MarkDeducedTemplateParameters` would be required again when we
decide to implement point 2.
I'm adding a FIXME anyway but leave the first intact. However, I'd be happy to
rearrange the logic flow if you insist.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156605/new/
https://reviews.llvm.org/D156605
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
