Make allocate @safe by edi33416 · Pull Request #5879 · dlang/phobos

edi33416 · 2017-11-21T14:08:20Z

For allocators that implement their own allocate method, this should be a pure nothrow @safe function.
Allocators that are building on top of such allocators should infer the function attributes from their parents.

This PR is a subset of #5330, as this approach will provide us with better granularity. More smaller PRs to come :)

dlang-bot · 2017-11-21T14:08:21Z

Thanks for your pull request, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

aG0aep6G · 2017-11-21T17:20:47Z

std/experimental/allocator/building_blocks/affix_allocator.d

+                static if (stateSize!Prefix)
+                {
+                    assert(result.ptr.alignedAt(Prefix.alignof));
+                    emplace!Prefix(cast(Prefix*) result.ptr);


This assumes that it's safe to write over result. As far as I see, you can't assume that. Every array type implicitly converts to void[], so it's possible that parent.allocate returns memory that's already used elsewhere, differently typed.

Same with Suffix below.

Isn't it safe to assume that parent.allocate will return fresh memory? Imho, allocate should always return fresh memory.

Isn't it safe to assume that parent.allocate will return fresh memory?

No. parent is provided by the user. You can't assume anything about it beyond what is guaranteed/required by the language.

It might be possible to check if the return value is unique. Then you can rely on that. But you have to check.

This is problematic. We may need to replace the result of allocate with ubyte[] everywhere, or give up on making allocate safe for this allocator.

andralex · 2017-11-30T03:12:00Z

@edi33416 I suggest for now let's leave AffixAllocator with an unsafe allocate.

wilzbach · 2017-11-21T17:33:07Z

std/experimental/allocator/building_blocks/affix_allocator.d


    // Check that goodAllocSize inherits from parent, i.e. GCAllocator
-    assert(__traits(compiles, (() nothrow @safe @nogc => a.goodAllocSize(1))()));
+    assert(__traits(compiles, (() @nogc => a.goodAllocSize(1))()));


This is an anti-pattern. Get rid of the __traits(compiles here.

@wilzbach What is the preferred alternative to __traits(compiles, ...)?

a.goodAllocSize(1) and if don't want to call it, simply put it in a function or lambda which isn't called. It looks a lot nicer to read and has the same effect and for the main reason you see the error messages e.g. if you decide to refactor your codebase.

FWIW it should have been static assert too.

wilzbach · 2017-11-21T17:33:38Z

std/experimental/allocator/building_blocks/affix_allocator.d

    auto b = a.allocate(42);
    assert(b.length == 42);
-    () nothrow @nogc { a.deallocate(b); }();
+    () @trusted @nogc { a.deallocate(b); }();


Where did the nothrow go? And better wrap the specific part in an @safe lambda than spreading @trusted

Where did the nothrow go?

I changed the unittest to nothrow @safe

And better wrap the specific part in an @safe lambda than spreading @trusted

What do you mean?

wilzbach · 2017-12-01T06:39:29Z

std/experimental/allocator/building_blocks/allocator_list.d

+            }
+
+            bool failedNewAlloc = false;
+            mixin(q{() } ~ trusted_ ~ q{ {


You can do this without a mixin, by simply doing sth. like

static if (<checkWhetherItCanBeTrusted>) () @trusted { callFun() }(); else callFun();

wilzbach · 2017-12-01T06:41:13Z

std/experimental/allocator/building_blocks/bitmapped_block.d

    metadata.
    */
-    @trusted void[] allocate(const size_t s)
+    pure nothrow @trusted @nogc


Why can this @trusted? (Please don't slap @trusted on functions.)

This was made @trusted before. I just added pure nothrow @nogc.
Should I make this @safe and add @trusted lambdas inside?

wilzbach · 2017-12-01T06:42:56Z

std/experimental/allocator/building_blocks/bucketizer.d

    /**
    Directs the call to either one of the $(D buckets) allocators.
    */
+    //pure nothrow @safe @nogc


wilzbach · 2017-12-01T06:49:25Z

std/experimental/allocator/building_blocks/free_tree.d

        static if (hasMember!(ParentAllocator, "deallocate"))
        {
-            clear;
+            () @trusted { clear; }();


How about making the function @safe instead of blindly trusting it?

wilzbach · 2017-12-01T06:49:50Z

std/experimental/allocator/building_blocks/free_tree.d

+            assert(b1.length == 10000);
+            assert(b2.length == 20000);
+            assert(b3.length == 30000);
+            () @trusted @nogc { a.deallocate(b1); }();


Close lambda and open it three lines later ;-)

wilzbach · 2017-12-01T06:54:50Z

std/experimental/allocator/building_blocks/quantizer.d

    assert(b.length == 42);
    // Ensure deallocate inherits from parent
-    () nothrow @nogc { a.deallocate(b); }();
+    () @trusted nothrow @nogc { a.deallocate(b); }();


You could have wrapped the other block in an @safe lambda ...

wilzbach · 2017-12-01T06:59:28Z

std/experimental/allocator/mmap_allocator.d

    version(Posix)
    {
        /// Allocator API.
+        @trusted @nogc nothrow


Don't apply @trusted to entire functions.

wilzbach · 2017-12-01T06:59:33Z

std/experimental/allocator/mmap_allocator.d

            PAGE_READWRITE, MEM_RELEASE;

        /// Allocator API.
+        @trusted @nogc nothrow


Don't apply @trusted to entire functions.

wilzbach · 2017-12-01T07:01:19Z

@edi33416 I recommend splitting this PR up into smaller chunks, s.t. it can be reviewed and merged more quickly. As an additional benefit your chances of running into merge conflicts decrease too ;-)

andralex · 2017-12-06T00:52:05Z

Per the convo with @edi33416 today, please convert this diff to making allocate only nothrow and (where applicable) @nogc.

wilzbach · 2018-03-30T19:16:30Z

Ping @edi33416

RazvanN7 · 2021-10-22T11:56:19Z

@edi33416 Any chance of finishing this?

LightBender · 2024-10-26T23:45:52Z

PR closed as stalled. If you wish to resurrect it you are welcome to do so. We will look into this in Phobos 3.

Make allocate safe

7f738d5

edi33416 requested review from PetarKirov, andralex and wilzbach as code owners November 21, 2017 14:08

aG0aep6G reviewed Nov 21, 2017

View reviewed changes

wilzbach reviewed Dec 1, 2017

View reviewed changes

dlang-bot added Review:Needs Work Merge:Needs Rebase labels Dec 29, 2017

This was referenced Apr 2, 2018

Make mmap_allocator allocate @safe #6405

Merged

Make null_allocator allocate @safe #6406

Merged

Make bitmapped_block allocate @safe #6407

Merged

dlang-bot added the Merge:stalled label May 27, 2021

dlang-bot removed the Merge:stalled label Oct 22, 2021

dlang-bot added the Merge:stalled label Feb 19, 2022

LightBender added the Review:Phantom Zone Has value/information for future work, but closed for now label Oct 26, 2024

LightBender closed this Oct 26, 2024

Uh oh!

Comments

Conversation

edi33416 commented Nov 21, 2017

Uh oh!

dlang-bot commented Nov 21, 2017

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Dec 1, 2017

Uh oh!

andralex commented Dec 6, 2017

Uh oh!

wilzbach commented Mar 30, 2018

Uh oh!

RazvanN7 commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LightBender commented Oct 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

RazvanN7 commented Oct 22, 2021 •

edited

Loading