Page MenuHomePhabricator

Fix overloaded static functions in SemaCodeComplete
ClosedPublic

Authored by yvvan on Aug 7 2017, 2:46 AM.

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=33904
Happens when static function is accessed via the class variable. That leads to incorrect overloads number because the variable is considered as the first argument.

struct Bar {

static void foo(); static void foo(int);

};

int main() {

Bar b;
b.foo(/*complete here*/); // did not work before
Bar::foo(/*complete here*/); // worked fine

}

Diff Detail

Repository
rL LLVM

Event Timeline

yvvan created this revision.Aug 7 2017, 2:46 AM
bkramer added inline comments.Aug 7 2017, 6:25 AM
include/clang/Sema/Sema.h
2681 ↗(On Diff #109961)

Shouldn't this be called with the new argument somehere? Otherwise it'll always be false.

yvvan updated this revision to Diff 109980.Aug 7 2017, 6:56 AM

Yes, I missed to include one file in this diff where it's used

yvvan added a comment.EditedAug 7 2017, 7:04 AM

Thought that I've just found a regression in my change.

But it's some unrelated behavior in fact - constructor work fine also

yvvan added a comment.Aug 8 2017, 5:18 AM

Please check that one

bkramer added inline comments.Aug 8 2017, 7:00 AM
include/clang/Sema/Sema.h
2681 ↗(On Diff #109980)

I'd prefer something like "FirstArgumentIsBase"

lib/Sema/SemaOverload.cpp
5871 ↗(On Diff #109980)

Unnecessary whitespace change.

6339 ↗(On Diff #109980)

Add a comment that the first argument is the base.

6341 ↗(On Diff #109980)

clang-format

6342 ↗(On Diff #109980)

assert that FD is a static method.

nik added a subscriber: nik.Aug 17 2017, 4:26 AM
nik added inline comments.
lib/Sema/SemaOverload.cpp
6342 ↗(On Diff #109980)

Just stumbled here because I was looking into https://bugs.llvm.org/show_bug.cgi?id=34207
Some observations

  • Slicing the first argument unconditionally in this branch (without the if at 6340) fixes the issue.
  • The current revision/version of this change does not fix the issue.

In case the crash is adapted with this patch, fine. Otherwise I'll wait until this one got in and look again at it.

yvvan added inline comments.Aug 17 2017, 4:32 AM
lib/Sema/SemaOverload.cpp
6342 ↗(On Diff #109980)

"Slicing the first argument unconditionally in this branch" is not valid because constructors are affected (at least them) where this argument is needed

yvvan updated this revision to Diff 111647.Aug 18 2017, 3:07 AM
yvvan marked 5 inline comments as done.

Review comments + solved https://bugs.llvm.org/show_bug.cgi?id=34207 issue.
I've also added the extra test for that issue

yvvan added a subscriber: erikjv.Aug 18 2017, 3:11 AM
yvvan added a comment.Sep 8 2017, 3:15 AM

one more ping :)

one more ping...

bkramer accepted this revision.Oct 26 2017, 12:26 AM

This looks good. Sorry for the long wait, do you have commit access?

This revision is now accepted and ready to land.Oct 26 2017, 12:26 AM

@bkramer Not yet, it would be good to have one though :)

This revision was automatically updated to reflect the committed changes.
cameron314 added inline comments.
cfe/trunk/lib/Sema/SemaOverload.cpp
6365

This breaks normal (non-static) method overload resolution, since the this argument is now passed to AddMethodCandidate which is not expecting it. It always thinks too many arguments are passed to methods with no parameters; most other calls to AddMethodCandidate in SemaOverload.cpp don't pass the implicit this.

cameron314 added inline comments.Nov 8 2017, 12:10 PM
cfe/trunk/lib/Sema/SemaOverload.cpp
6396

The same slice that was added above needs to be done here for templated static methods that are accessed via a non-static object.

yvvan added inline comments.Nov 21 2017, 1:16 AM
cfe/trunk/lib/Sema/SemaOverload.cpp
6365

This code is correct. It is sliced at line 6361 - only in the case when the args size is greater than 0.

6396

It might be the case. You can add that :)
I did not check template cases in details.

cameron314 added inline comments.Dec 4 2017, 3:16 PM
cfe/trunk/lib/Sema/SemaOverload.cpp
6365

Hmm, you're right, I didn't see that. That line was missing after a rebase on our side, my fault for not properly cross-referencing the diffs. Sorry for the false alarm.

But shouldn't the slice only be done now when Args.size() > 0 && (!Args[0] || (FirstArgumentIsBase && isa<CXXMethodDecl>(FD) && !isa<CXXConstructorDecl>(FD)))?

6396

It is the case, I have parameter completion tests that fail without this :-)
I'll commit in a few days.

yvvan added inline comments.Dec 5 2017, 2:20 AM
cfe/trunk/lib/Sema/SemaOverload.cpp
6365

there's already a check for this case that it's a CXXMethodDecl, it's not static. That means that we always need to slice (except those rare cases when we don't have any args, and in fact I don't know why these cases exist)

nik added a comment.Apr 17 2018, 1:08 AM

Huch, seems already submitted. Ignore :>