Add new builtin trait isCopyable #10575

nordlow · 2019-11-16T22:02:53Z

I'm using std.traits.isCopyable all over the place to make my containers as generic as possible. So why not reuse the already existing isCopyable() in mtype.d to further optimize compilation.

dlang-bot · 2019-11-16T22:02:54Z

Thanks for your pull request and interest in making D better, @nordlow! 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 coverage diff by visiting the details link of the codecov check)
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 run digger -- build "master + dmd#10575"

thewilsonator · 2019-11-16T22:23:49Z

Needs a changelog.

nordlow · 2019-11-16T22:44:12Z

Done.

src/dmd/traits.d

SSoulaimane · 2019-11-17T05:55:49Z

changelog/add_traits_isCopyable.dd

@@ -0,0 +1,5 @@
+Add new builtin `__traits(isCopyable, T)` for some type `T`
+
+Changing Phobos' `std.traits.isCopyable` to make use of this new builtin trait


The function mtype.isCopyable() in its current form may yield different results from std.traits.isCopyable since it doesn't check the copy constructor.

import std; struct S { @disable this(ref S); } pragma(msg, isCopyable!S); // false pragma(msg, __traits(isCopyable, S)); // true

Do you have a suggestion for how to check for a disabled copy constructor? Should I copy the logic from e.ident == Id.hasCopyConstructor?

Further, should we add this copy constructor checking directly into isCopyable in mtype.d or only inside the new if-statement in traits.d?

You need to check if there is a matching copy constructor overload for your type. It should be inside isCopyable.
This should do if you remove the nothrow pure @nogc attributes.

if (ts.sym.hasCopyCtor) { // check if there is a matching overload of the copy constructor and whether it is disabled or not Dsymbol ctor = search_function(cast() ts.sym, Id.This); assert(ctor); scope el = new IdentifierExp(Loc.initial, Id.p); // dummy lvalue el.type = cast() ts; Expressions args; args.push(el); FuncDeclaration f = resolveFuncCall(Loc.initial, null, ctor, null, cast()ts, &args, FuncResolveFlag.quiet); if (!f || f.storage_class & STC.disable) return false; }

Done. Added a matching test aswell.

FuncDeclaration f = resolveFuncCall(Loc.initial, null, ctor, null, cast()ts, &args, FuncResolveFlag.quiet);

Is there now direct way of looking up the specific constructor that takes ref typeof(this) as single parameter?

I guess you want to cache the result instead of trying every overload every time. I don't think it's done yet. If you want to do it, it has to be stored on the TypeStruct instead of the StructDeclaration because there can be multiple types with different type modifiers (const, immutable, shared... etc) which all share the same StructDeclaration but each will match a different overload of the copy constructor.

Can we add this caching in another pull request?

Since it's not required, yes.

src/dmd/traits.d

WalterBright · 2019-11-17T07:47:42Z

Here's the library implementation:
https://github.com/dlang/phobos/blob/master/std/traits.d#L8832

enum isCopyable(S) = is(typeof(
    { S foo = S.init; S copy = foo; }
));

I'm not really seeing much point it building it in to the compiler. Also, this PR should include the tests from the Phobos version.

nordlow · 2019-11-24T22:32:37Z

Here's the library implementation:
https://github.com/dlang/phobos/blob/master/std/traits.d#L8832
enum isCopyable(S) = is(typeof(
    { S foo = S.init; S copy = foo; }
));
I'm not really seeing much point it building it in to the compiler. Also, this PR should include the tests from the Phobos version.

Yes, this is a microoptimization for eliding parsing of the is(typeof(...))-expression and the template instantiation (when using this __traits directly) but in the long run all these add up to measurable speed-ups. Do you want me to perform a benchmark comparing this new trait to the existing std.traits.isCopyable?

nordlow · 2019-12-01T18:13:14Z

auto-tester passes but in

https://circleci.com/gh/dlang/dmd/32501?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

line 6768 containing

assert(ctor);

fails. Should this assert be replaced by

if (ctor is null) { return true; }

?

SSoulaimane · 2019-12-01T21:56:01Z

The assertion fails because I made a mistake in the code snippet, it should be search_function(ts.sym, Id.ctor).

nordlow · 2019-12-01T22:59:36Z

The assertion fails because I made a mistake in the code snippet, it should be search_function(ts.sym, Id.ctor).

Done.

nordlow · 2019-12-02T08:59:38Z

Ready to merge.

nordlow · 2019-12-03T11:50:02Z

I can't figure out why the Windows_OMF x64 build fails.

MoonlightSentinel · 2019-12-03T12:28:56Z

I can't figure out why the Windows_OMF x64 build fails.

Seems like the CI does not like your name ... /s

FromExe:
...
BUILD_SOURCEVERSIONAUTHOR=Per Nordlöw

...
FromRun:
...
BUILD_SOURCEVERSIONAUTHOR=Per Nordl”w

nordlow · 2019-12-03T12:35:59Z

So what to do?

wilzbach · 2019-12-03T13:03:27Z

It's not a required CI, so maintainers can just force-merge it if this gets approved.
Alternatively, the relevant test is:

https://github.com/dlang/dmd/blob/master/test/dshell/sameenv.d
https://github.com/dlang/dmd/blob/master/test/dshell/extra-files/printenv.d

You could probably add a filter for BUILD_SOURCEVERSIONAUTHOR in printenv.d?

nordlow · 2019-12-03T13:15:20Z

This has been approved by @SSoulaimane

nordlow · 2019-12-03T16:10:21Z

It's not a required CI, so maintainers can just force-merge it if this gets approved.
Alternatively, the relevant test is:

https://github.com/dlang/dmd/blob/master/test/dshell/sameenv.d
https://github.com/dlang/dmd/blob/master/test/dshell/extra-files/printenv.d

You could probably add a filter for BUILD_SOURCEVERSIONAUTHOR in printenv.d?

Are you saying I could try modifying this code to support names containing the letter ö?

wilzbach · 2019-12-03T16:29:41Z

Are you saying I could try modifying this code to support names containing the letter ö?

Well, while that would the ideal solution, I said either ignoring the failure or filtering the respective environment variable out should be fine as well.

This has been approved by @SSoulaimane

Yes, but Walter's objection hasn't been addressed.

nordlow · 2019-12-03T19:05:41Z

Do you still oppose adding this, @WalterBright?

12345swordy · 2020-02-23T21:58:10Z

I think benchmarks before and after this PR would be very persuasive in convincing @WalterBright to pull this request.

12345swordy · 2020-03-09T21:13:17Z

ping @nordlow

nordlow · 2020-03-10T10:14:48Z

I think benchmarks before and after this PR would be very persuasive in convincing @WalterBright to pull this request.

What kinds of benchmarks do you propose? Iterating over a large set of types via static foreach and applying the trait to all of them to make the variation in compilation time noticeable?

12345swordy · 2020-03-10T13:30:40Z

I think benchmarks before and after this PR would be very persuasive in convincing @WalterBright to pull this request.

What kinds of benchmarks do you propose? Iterating over a large set of types via static foreach and applying the trait to all of them to make the variation in compilation time noticeable?

I propose that you create a compile speed benchmark of the the trait "IsCopyable" when implement via library vs built in trait that your PR Contain. You need to show that there are significant improvements when built into the compiler.

12345swordy

This needs benchmark test, otherwise lgtm.

nordlow · 2020-05-24T18:23:17Z

Thanks!

WalterBright · 2020-05-24T22:20:15Z

No benchmark is done, which is odd because in the opening statement here you had a project that used it a lot, but there's no mention of what (if any) performance gains were realized on it.

The point of benchmarking is to figure out which of std.traits are worthwhile to find faster solutions to. There are a lot of them. Doing them randomly is not very practical. As far as I can see, there is no rationale for doing isCopyable.

nordlow · 2020-05-28T22:40:30Z

@WalterBright:

The following contrived benchmark

import std.traits : isCopyable;
import std.meta : AliasSeq;

struct W(T, size_t n)
{
    T value;
}

@safe pure unittest
{
    alias Ts(uint n) = AliasSeq!(W!(byte, n), W!(ubyte, n),
                                 W!(short, n), W!(ushort, n),
                                 W!(int, n), W!(uint, n),
                                 W!(long, n), W!(ulong, n),
                                 W!(float, n), W!(cfloat, n),
                                 W!(double, n), W!(cdouble, n),
                                 W!(real, n), W!(creal, n),
                                 W!(string, n), W!(wstring, n), W!(dstring, n));

    enum n = 100;
    enum m = 100;
    static foreach (i; 0 .. n)
    {
        foreach (T; Ts!(n))
        {
            static foreach (j; 0 .. m)
            {
                static assert(__traits(isCopyable, T));
                // static assert(isCopyable!(T));
            }
        }
    }
}

semantically checks in 0.9 s with __traits(isCopyable, T) compared to 1.1 s when instead isCopyable!(T) is used. So at least the improvement in compilation is measureable. Do you need anything more than this Walter? For more detailed analysis I presume dmdprod could be of service.

On pro of adding this builtin trait and reusing it in the implementation of std.traits.isCopyable is to collect the logic of checking for copyability in one place that can be reused both by the compiler and by std.traits.

WalterBright · 2020-05-29T05:48:25Z

Thank you, Per.

CyberShadow · 2021-06-26T01:17:28Z

@nordlow Could you please also make a PR to dlang.org which adds this to the language specification / grammar?

nordlow · 2021-06-26T17:40:09Z

@nordlow Could you please also make a PR to dlang.org which adds this to the language specification / grammar?

Yep. See dlang/dlang.org#3025

Add new builtin trait isCopyable

4d88409

Add changelog

823db15

Simplify header

10cbbc3

SSoulaimane reviewed Nov 17, 2019

View reviewed changes

Add more checks and tests

9ac5d6f

nordlow added 3 commits November 28, 2019 11:25

Add checks for disabled copy constructor in isCopyable

e11b1e1

Remove assert on return value of search_function

9bcdf83

Put back assert(ctor)

129cfde

Change Id.This to Id.ctor

eb7cefb

SSoulaimane approved these changes Dec 1, 2019

View reviewed changes

MoonlightSentinel mentioned this pull request Jan 9, 2020

Fix issue 20478 dlang/phobos#7342

Closed

Merge remote-tracking branch 'upstream/master' into isCopyable-trait

a5f95ba

12345swordy approved these changes Mar 21, 2020

View reviewed changes

thewilsonator added the auto-merge label May 21, 2020

dlang-bot merged commit 93b1ca8 into dlang:master May 21, 2020

rmanthorpe mentioned this pull request Jul 21, 2020

Fix Issue 12223 - __traits(getMember,...) needed for aliases #11442

Merged

CyberShadow mentioned this pull request Jun 26, 2021

Ensure all DMD test/compilable tests parse successfully CyberShadow/tree-sitter-d#2

Closed

nordlow deleted the isCopyable-trait branch October 19, 2021 19:43

		@@ -0,0 +1,5 @@
		Add new builtin `__traits(isCopyable, T)` for some type `T`

		Changing Phobos' `std.traits.isCopyable` to make use of this new builtin trait

Add new builtin trait isCopyable #10575

Add new builtin trait isCopyable #10575

Conversation

nordlow commented Nov 16, 2019 • edited Loading

dlang-bot commented Nov 16, 2019 • edited Loading

Bugzilla references

Testing this PR locally

thewilsonator commented Nov 16, 2019

nordlow commented Nov 16, 2019

SSoulaimane Nov 17, 2019

Choose a reason for hiding this comment

nordlow Nov 24, 2019 • edited Loading

Choose a reason for hiding this comment

SSoulaimane Nov 25, 2019

Choose a reason for hiding this comment

nordlow Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

nordlow Nov 28, 2019

Choose a reason for hiding this comment

SSoulaimane Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

nordlow Dec 1, 2019 • edited Loading

Choose a reason for hiding this comment

SSoulaimane Dec 1, 2019

Choose a reason for hiding this comment

WalterBright commented Nov 17, 2019

nordlow commented Nov 24, 2019 • edited Loading

nordlow commented Dec 1, 2019 • edited Loading

SSoulaimane commented Dec 1, 2019 • edited Loading

nordlow commented Dec 1, 2019

nordlow commented Dec 2, 2019

nordlow commented Dec 3, 2019

MoonlightSentinel commented Dec 3, 2019

nordlow commented Dec 3, 2019

wilzbach commented Dec 3, 2019

nordlow commented Dec 3, 2019

nordlow commented Dec 3, 2019 • edited Loading

wilzbach commented Dec 3, 2019

nordlow commented Dec 3, 2019

12345swordy commented Feb 23, 2020

12345swordy commented Mar 9, 2020

nordlow commented Mar 10, 2020 • edited Loading

12345swordy commented Mar 10, 2020

12345swordy left a comment

Choose a reason for hiding this comment

nordlow commented May 24, 2020

WalterBright commented May 24, 2020

nordlow commented May 28, 2020 • edited Loading

WalterBright commented May 29, 2020

CyberShadow commented Jun 26, 2021

nordlow commented Jun 26, 2021

nordlow commented Nov 16, 2019 •

edited

Loading

dlang-bot commented Nov 16, 2019 •

edited

Loading

nordlow Nov 24, 2019 •

edited

Loading

nordlow Nov 28, 2019 •

edited

Loading

SSoulaimane Nov 28, 2019 •

edited

Loading

nordlow Dec 1, 2019 •

edited

Loading

nordlow commented Nov 24, 2019 •

edited

Loading

nordlow commented Dec 1, 2019 •

edited

Loading

SSoulaimane commented Dec 1, 2019 •

edited

Loading

nordlow commented Dec 3, 2019 •

edited

Loading

nordlow commented Mar 10, 2020 •

edited

Loading

nordlow commented May 28, 2020 •

edited

Loading