Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

[WIP] Add @nogc array#2348

Closed
edi33416 wants to merge 22 commits intodlang:masterfrom
edi33416:add_nogc_array
Closed

[WIP] Add @nogc array#2348
edi33416 wants to merge 22 commits intodlang:masterfrom
edi33416:add_nogc_array

Conversation

@edi33416
Copy link
Contributor

This PR adds a @nogc nothrow pure @safe implementation of a generic array that mimics the behaviour of the built-in array (T[]).

This array implementation makes use of the Copy Constructor PR. By doing so, it both tests the implementation and proves it's usefulness.

I know this is a lengthy PR, but please bear with me, as there are a lot of unittests.
And now, as Andrei says "destroy()"

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @edi33416! 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 + druntime#2348"

@dlang-bot dlang-bot added the WIP Work In Progress - not ready for review or pulling label Nov 11, 2018
enum classInstanceAlignment(T) = size_t.alignof >= T.alignof ? size_t.alignof : T.alignof;

T emplace(T, Args...)(T chunk, auto ref Args args)
if (is(T == class))
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to handle non D classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. What non D classes are supported? C++ ones?

Copy link
Contributor

@jacob-carlborg jacob-carlborg Nov 12, 2018

Choose a reason for hiding this comment

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

In addition to C++ classes: Objective-C classes and interfaces and COM interfaces.

Copy link
Member

Choose a reason for hiding this comment

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

@jacob-carlborg that was copied from the phobos implementation (later to be moved), can you explain what needs to be done here? Thanks! Also: would rather add that as an additional PR after this.

Copy link
Contributor

@jacob-carlborg jacob-carlborg Nov 12, 2018

Choose a reason for hiding this comment

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

TypeInfo is somewhat broken for non D classes. But emplace might not be affected. Test it and see what happens and make sure to add tests for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be private then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you guys think about moving emplace to core.internal.traits ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really a trait is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. My bad, sorry :).
The question remains: should we move it into a core.internal.conv ?

I think emplace is especially usefull when you want to construct an immutable object into a predefined memory area.

Copy link
Member

Choose a reason for hiding this comment

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

Let's make it private for now and postpone that decision. Fight one fight at a time.

}

void dispose(A, T)(auto ref A alloc, auto ref T p)
if (is(T == class) || is(T == interface))
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to handle non D classes and interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same question: can you please provide me with an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Replied to the other comment.

a.front.x = 20;
}
assert(c.x == 20);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should have tests with non D classes as well.

@jacob-carlborg
Copy link
Contributor

  • There’s a lot of code related to traits, do we want that in the same module?
  • Why are some range functions included and why are they in the Array struct?

@jacob-carlborg
Copy link
Contributor

@edi33416
Copy link
Contributor Author

* There’s a lot of code related to traits, do we want that in the same module?

There is, and I want to remove as much as I can. I didn't know about core.internal.traits until like 10mins ago.
Regarding core.internal.traits, I see there is a duplicate of the Phobos Unqual there. I'll open a PR in Phobos that will alias to core.internal.traits.Unqual, so we won't have to maintain the same code twice.

* Why are some range functions included and why are they in the Array struct?

I needed those for some of the template constraints and other in order to consume T[]

@jacob-carlborg
Copy link
Contributor

I needed those for some of the template constraints and other in order to consume T[]

If they’re only for implementation they should be private.

@edi33416
Copy link
Contributor Author

I'll open a PR in Phobos that will alias to core.internal.traits.Unqual, so we won't have to maintain the same code twice.

Said PR.

@andralex
Copy link
Member

TODO: rename module to rcslice.d and type name with rcslice.

@andralex
Copy link
Member

All: would rcarray be preferable to rcslice?

setIsShared(support, true);
}

this(immutable ref typeof(this) rhs) immutable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing to
this(scope immutable ref typeof(this) rhs) immutable fix the scope error, but will make the ctor @system.
Adding a @trusted (For testing purposes only) to quickly mute all the complaints will pass all the tests. Why is the scope required and why is it making the function @system when used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another way of bypassing the error, without any @trusted "magic" is to change the idup implementation from

immutable(rcarray!T) idup(this Q)()
{
    return immutable rcarray!T(this);
}

to

immutable(rcarray!T) idup(this Q)()
{
    static if (is(Q == immutable))
    {
        return this;
    }
    else
    {
        return immutable rcarray!T(this);
    }
}

@anon17
Copy link

anon17 commented Apr 22, 2019

The is condition in PrefixAllocator is always true.

alias localAllocator = shared PrefixAllocator.instance;
alias sharedAllocator = shared PrefixAllocator.instance;

shared looks out of place here.

lesderid added a commit to lesderid/druntime that referenced this pull request Jun 18, 2019
Squashed commit of the following:

commit ee96796
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Fri Mar 29 15:16:46 2019 +0200

    Fix invalid support bug

commit 8e2a813
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Thu Dec 6 18:21:57 2018 +0200

    Make array usable in betterC

commit ab1eef7
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Dec 4 16:29:00 2018 +0200

    Leave bounds checking to underlying T[] support

commit 82b1bbc
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Dec 4 16:28:04 2018 +0200

    Mark isShared as the msb inside the ref count

commit 4410979
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Thu Nov 29 14:14:54 2018 +0200

    Use version (CoreUnittest) for stats allocator

commit d0a8ae5
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 27 13:52:22 2018 +0200

    Refactor idup

commit a144462
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 27 13:50:26 2018 +0200

    Refactor dup

commit 97db995
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 17:24:30 2018 +0200

    Rename to `rcarray`

commit 4ac6812
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 17:15:46 2018 +0200

    Comply with D_NoBoundsCheck

commit 5d98a65
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 16:26:41 2018 +0200

    DStyle: Add space between version (

commit 68191fa
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 16:17:44 2018 +0200

    Make `each` compatible with Phobos' `each`

commit 65c0fde
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 21:05:08 2018 +0000

    Change from labels to inline access specifiers

commit 5fa4cbd
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 16:48:24 2018 +0200

    Remove array's internal range API

commit 025b50f
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 15:25:05 2018 +0200

    Make global symbols private

commit 86ea7ed
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 14:53:57 2018 +0200

    Implement opEquals and remove global equals fn

commit 60b3143
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 14:27:06 2018 +0200

    Allow toHash to infer safety

commit 815942b
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 14:23:22 2018 +0200

    Remove obsolete ctor

commit 89b48a6
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 22:01:55 2018 +0000

    Remove range deps

commit d9c02b0
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 15:34:10 2018 +0000

    Rename internal fields

commit 6642656
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 15:07:28 2018 +0000

    Import Unqual from core traits

commit 039483d
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 15:05:52 2018 +0000

    Make length behave as builtin array's

commit a206494
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Sun Nov 11 21:46:11 2018 +0000

    [WIP] Add @nogc array
lesderid added a commit to lesderid/druntime that referenced this pull request Jun 24, 2019
This commit fixes rcarray for class types, storing class references
(i.e. not class states).

The previous code (both in dlang#2348
and in https://github.com/dlang-stdx/collections) was broken and
allocated memory for a class *state* but used the memory to store
*references*.
lesderid added a commit to lesderid/druntime that referenced this pull request Jul 5, 2019
Squashed commit of the following:

commit ee96796
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Fri Mar 29 15:16:46 2019 +0200

    Fix invalid support bug

commit 8e2a813
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Thu Dec 6 18:21:57 2018 +0200

    Make array usable in betterC

commit ab1eef7
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Dec 4 16:29:00 2018 +0200

    Leave bounds checking to underlying T[] support

commit 82b1bbc
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Dec 4 16:28:04 2018 +0200

    Mark isShared as the msb inside the ref count

commit 4410979
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Thu Nov 29 14:14:54 2018 +0200

    Use version (CoreUnittest) for stats allocator

commit d0a8ae5
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 27 13:52:22 2018 +0200

    Refactor idup

commit a144462
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 27 13:50:26 2018 +0200

    Refactor dup

commit 97db995
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 17:24:30 2018 +0200

    Rename to `rcarray`

commit 4ac6812
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 17:15:46 2018 +0200

    Comply with D_NoBoundsCheck

commit 5d98a65
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 16:26:41 2018 +0200

    DStyle: Add space between version (

commit 68191fa
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 16:17:44 2018 +0200

    Make `each` compatible with Phobos' `each`

commit 65c0fde
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 21:05:08 2018 +0000

    Change from labels to inline access specifiers

commit 5fa4cbd
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 16:48:24 2018 +0200

    Remove array's internal range API

commit 025b50f
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 15:25:05 2018 +0200

    Make global symbols private

commit 86ea7ed
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 14:53:57 2018 +0200

    Implement opEquals and remove global equals fn

commit 60b3143
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 14:27:06 2018 +0200

    Allow toHash to infer safety

commit 815942b
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 14:23:22 2018 +0200

    Remove obsolete ctor

commit 89b48a6
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 22:01:55 2018 +0000

    Remove range deps

commit d9c02b0
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 15:34:10 2018 +0000

    Rename internal fields

commit 6642656
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 15:07:28 2018 +0000

    Import Unqual from core traits

commit 039483d
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 15:05:52 2018 +0000

    Make length behave as builtin array's

commit a206494
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Sun Nov 11 21:46:11 2018 +0000

    [WIP] Add @nogc array
lesderid added a commit to lesderid/druntime that referenced this pull request Jul 5, 2019
This commit fixes rcarray for class types, storing class references
(i.e. not class states).

The previous code (both in dlang#2348
and in https://github.com/dlang-stdx/collections) was broken and
allocated memory for a class *state* but used the memory to store
*references*.
lesderid added a commit to lesderid/druntime that referenced this pull request Jul 5, 2019
Squashed commit of the following:

commit ee96796
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Fri Mar 29 15:16:46 2019 +0200

    Fix invalid support bug

commit 8e2a813
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Thu Dec 6 18:21:57 2018 +0200

    Make array usable in betterC

commit ab1eef7
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Dec 4 16:29:00 2018 +0200

    Leave bounds checking to underlying T[] support

commit 82b1bbc
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Dec 4 16:28:04 2018 +0200

    Mark isShared as the msb inside the ref count

commit 4410979
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Thu Nov 29 14:14:54 2018 +0200

    Use version (CoreUnittest) for stats allocator

commit d0a8ae5
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 27 13:52:22 2018 +0200

    Refactor idup

commit a144462
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 27 13:50:26 2018 +0200

    Refactor dup

commit 97db995
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 17:24:30 2018 +0200

    Rename to `rcarray`

commit 4ac6812
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 17:15:46 2018 +0200

    Comply with D_NoBoundsCheck

commit 5d98a65
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 16:26:41 2018 +0200

    DStyle: Add space between version (

commit 68191fa
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 26 16:17:44 2018 +0200

    Make `each` compatible with Phobos' `each`

commit 65c0fde
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 21:05:08 2018 +0000

    Change from labels to inline access specifiers

commit 5fa4cbd
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 16:48:24 2018 +0200

    Remove array's internal range API

commit 025b50f
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 15:25:05 2018 +0200

    Make global symbols private

commit 86ea7ed
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 14:53:57 2018 +0200

    Implement opEquals and remove global equals fn

commit 60b3143
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 14:27:06 2018 +0200

    Allow toHash to infer safety

commit 815942b
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Tue Nov 20 14:23:22 2018 +0200

    Remove obsolete ctor

commit 89b48a6
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 22:01:55 2018 +0000

    Remove range deps

commit d9c02b0
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 15:34:10 2018 +0000

    Rename internal fields

commit 6642656
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 15:07:28 2018 +0000

    Import Unqual from core traits

commit 039483d
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Mon Nov 12 15:05:52 2018 +0000

    Make length behave as builtin array's

commit a206494
Author: Eduard Staniloiu <edi33416@gmail.com>
Date:   Sun Nov 11 21:46:11 2018 +0000

    [WIP] Add @nogc array
lesderid added a commit to lesderid/druntime that referenced this pull request Jul 5, 2019
This commit fixes rcarray for class types, storing class references
(i.e. not class states).

The previous code (both in dlang#2348
and in https://github.com/dlang-stdx/collections) was broken and
allocated memory for a class *state* but used the memory to store
*references*.
@dlang-bot dlang-bot added Needs Rebase needs a `git rebase` performed Needs Work stalled labels May 27, 2021
@RazvanN7 RazvanN7 closed this Dec 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Needs Rebase needs a `git rebase` performed Needs Work stalled WIP Work In Progress - not ready for review or pulling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants