Skip to content

fix issue 17177. AutoImplement fails on function overload sets with …#5119

Merged
DmitryOlshansky merged 3 commits intodlang:masterfrom
aermicioi:issue_17177
Feb 21, 2017
Merged

fix issue 17177. AutoImplement fails on function overload sets with …#5119
DmitryOlshansky merged 3 commits intodlang:masterfrom
aermicioi:issue_17177

Conversation

@aermicioi
Copy link
Contributor

Issue: https://issues.dlang.org/show_bug.cgi?id=17177
Problem found in generation of "parent" that is used to reference super.member in AutoImplement template.
Fixed it by, aliasing parent to getMember trait that gets super.member, instead of assigning parent to super.member delegate.

…cannot infer type from overloaded function symbol".
@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
17177 AutoImplement fails on function overload sets with "cannot infer type from overloaded function symbol"

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a valid fix at the first glance. Thanks a lot!
However, as my CTFE-foo is limited maybe @UplinkCoder can spare a pair of eyes?

if (WITH_BASE_CLASS && !__traits(isAbstractFunction, func))
//preamble ~= "alias super." ~ name ~ " parent;\n"; // [BUG 2540]
preamble ~= "auto parent = &super." ~ name ~ ";\n";
preamble ~= "alias parent = AliasSeq!(__traits(getMember, super, \"" ~ name ~ "\"))[0];";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing that catches my eye is that AliasSeq!(...)[0] pattern - is it required?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's to work around a grammar limitation with alias = __traits. The compiler will complain something like "basic type expected, not __traits" and the identity template (which AliasSeq is) works around it, and the [0] kills the tupleness introduced by it.

I've used the pattern myself many times.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A solution would be to use Identity alias from std.meta. The problem is that it is private for some reason. I'd say __traits would look nicer being encapsulated in Identity alias rather than AliasSeq template.

Copy link
Contributor

@adamdruppe adamdruppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Code seems fine, passes the new test, doesn't fail any others = win.

@adamdruppe
Copy link
Contributor

I don't know what's up with that codecov thing, the Details it links to appear nonsensical for this diff.

@wilzbach
Copy link
Contributor

I don't know what's up with that codecov thing, the Details it links to appear nonsensical for this diff.

FYI:

  1. DMD doesn't emit code coverage information for CTFE. Hence codecov/patch isn't useful for this PR
  2. CircleCi doesn't support PRs which are merged into upstream/master on the CI. Hence codecov/project compares to an invalid "base" (see https://github.com/codecov/support/issues/360 for more details)

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

@wilzbach
Copy link
Contributor

@DmitryOlshansky only approved PRs can be merged, hence the message:

Merging is blocked

@DmitryOlshansky DmitryOlshansky merged commit ef0dffa into dlang:master Feb 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants