Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

dup, idup, keys, values: if a type is known to have a postblit do not emit code for the non-postblit path and vice versa #3034

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented Apr 16, 2020

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
20741 enhancement dup, idup for arrays plus keys, values for built-in associative arrays: if a type is known to have a postblit do not emit code for the non-postblit path and vice versa

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 + druntime#3034"

@dlang-bot dlang-bot added the Enhancement New functionality label Apr 16, 2020
@kinke
Copy link
Contributor

kinke commented Apr 16, 2020

Hmm, this doesn't seem to consider copy constructors at all. This should probably be refactored to use core.lifetime.emplace for copy-construction, which seems to correctly handle both cases:

import core.lifetime, core.stdc.stdio;

struct WithPostBlit
{
    int a = 42;
    this(int a) { this.a = a; }
    this(this) { printf("postblit: %d\n", a); }
}

struct WithCopyCtor
{
    int a = 42;
    this(int a) { this.a = a; }
    this(const ref WithCopyCtor rhs) { printf("copy ctor: %d => %d\n", a, rhs.a); }
}

void main()
{
    static void test(T)()
    {
    	T src = T(123);
        T dst = void;
        emplace(&dst, src);
    }

    test!WithPostBlit();
    test!WithCopyCtor();
}

=>

postblit: 123
copy ctor: 42 => 123

@n8sh
Copy link
Member Author

n8sh commented Apr 19, 2020

That sounds right to me although making it work with keys and values would require more extensive changes since the arrays are built by the opaque functions rt.aaA._aaKeys and rt.aaA._aaValues which are non-templated and instead use TypeInfo. Touching up the arrays afterwards works for a postblit but not for a copy constructor.

Possible solutions:

  • Expose the structure of built-in AAs so the object module can traverse them directly.
  • Add a copyConstructor(void* src, void* dst) function to TypeInfo and change rt.aaA._aaKeys and rt.aaA._aaValues to call the copy constructor on their side.
  • Add variants of rt.aaA._aaKeys and rt.aaA._aaValues that accept a copy constructor parameter and call it on their side.

@kinke
Copy link
Contributor

kinke commented Apr 19, 2020

Yeah I know that it's not directly related to this PR, it just stood out when looking at this code. There are surely more places where the copy ctor is ignored, incl. the compiler itself.

@thewilsonator
Copy link
Contributor

grep -nrE "\<(for|foreach|foreach_reverse|if|while|switch|catch|version)\(" $(find src -name '*.d') ; test $? -eq 1
src/object.d:2535:    static if(__traits(hasPostblit, Value))
posix.mak:440: recipe for target 'style_lint' failed
-    static if(__traits(hasPostblit, Value))
+    static if (__traits(hasPostblit, Value))

…built-in associative arrays: if a type is known to have a postblit do not emit code for the non-postblit path and vice versa
@dlang-bot dlang-bot merged commit bd67a95 into dlang:master Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants