Skip to content

fixup for enhancement 7804 - allow __traits(getMember) as basic type#8945

Merged
dlang-bot merged 1 commit intomasterfrom
unknown repository
Nov 21, 2018
Merged

fixup for enhancement 7804 - allow __traits(getMember) as basic type#8945
dlang-bot merged 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Nov 11, 2018

Addresses the problems detected in #8938 after it's been merged by error.

@ghost ghost requested a review from RazvanN7 as a code owner November 11, 2018 16:32
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @bbasile! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8945"

Geod24
Geod24 previously requested changes Nov 12, 2018
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Special-casing getMember has all the smells of bad design. The following just needs to work:

class Foo
{
    void func() {}
}
alias Truc = __traits(getOverloads, Foo, __traits(allMembers, Foo)[0])[0];
  1. Having the special casing as a parse error is a bad idea. Consider:
class Foo
{
    void func() {}
}

version (none)
    alias Truc = __traits(getOverloads, Foo, __traits(allMembers, Foo)[0])[0];

This errors out. I really don't think it should.

@TurkeyMan
Copy link
Contributor

Thanks so much for resurrecting this PR!

@ghost ghost requested review from dnadlinger and ibuclaw as code owners November 12, 2018 15:33
@ghost
Copy link
Author

ghost commented Nov 14, 2018

Without special handling, the overload set obtained from getOverloads is resolved to a tuple made of the type of each overload. How do you convert a TupleExp to a DSymbol ?

@thewilsonator
Copy link
Contributor

From the example above it would seem that you (well the users code) needs to index the TupleExp probably yielding something like a SymExp(?).

@ghost
Copy link
Author

ghost commented Nov 15, 2018

TupleDeclaration seems to do the job actually. Just gotta figure out how unique identifiers are created.

@ghost
Copy link
Author

ghost commented Nov 15, 2018

The PR is ready for a second review i think. getOverloads is also supported so the feature is decently usable now.

@ghost
Copy link
Author

ghost commented Nov 16, 2018

(fixed) Sadly there's a performance regression when it's used in place of AliasSeq.
For example

module bench;

import std.meta : AliasSeq;

class Foo(int number){}
enum Bar(int number) = 8;
class Class(int number){}

template Stuff(int number)
{
    @Foo!number @Bar!number @(number) class ClassN : Class!number {}
    version(new_way) alias Attribs = __traits(getAttributes, ClassN);
    else alias Attribs = AliasSeq!(__traits(getAttributes, ClassN));
}

enum int count = 10_000;

static foreach (immutable(int) i; 0 .. count)
    mixin Stuff!i;

void main(){}

gives

$ time ./dmd bench.d
real    0m12,850s
user    0m12,186s
sys     0m0,637s

$ time ./dmd bench.d -version=new_way
real    0m25,344s
user    0m24,578s
sys     0m0,679s

@thewilsonator
Copy link
Contributor

Seems like a good candidate for https://github.com/CyberShadow/dmdprof although I'm not particularly convinced thats a typical usage pattern, but a 2x slowdown isn't exactly negligible either.

Copy link
Member

@UplinkCoder UplinkCoder left a comment

Choose a reason for hiding this comment

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

A change of this scale will need a DIP, I think.

@ghost
Copy link
Author

ghost commented Nov 18, 2018

@UplinkCoder, it was more or less preapproved by @WalterBright here. Although i won't get mad if it has to be done, now that the implementation is finished.

@thewilsonator
Copy link
Contributor

@bbasile is there anything more to be done?

@thewilsonator thewilsonator dismissed Geod24’s stale review November 18, 2018 05:15

No special casing

@ghost
Copy link
Author

ghost commented Nov 18, 2018

Maybe i'll feel the need to push more tests but otherwise, unless changes required by future reviewers, it's good.

@thewilsonator thewilsonator added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Nov 18, 2018
Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

Nice work @bbasile!

@thewilsonator thewilsonator added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Nov 21, 2018
@dlang-bot dlang-bot merged commit b30881f into dlang:master Nov 21, 2018
@ghost ghost deleted the fixup-alias-equal-traits branch November 21, 2018 08:48
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.

7 participants