On 6/13/15 4:16 PM, ZombineDev wrote:
> On Saturday, 13 June 2015 at 15:48:31 UTC, Andrei Alexandrescu wrote:
>> On 6/13/15 3:14 AM, Dicebot wrote:
>>> Andrei, have you considered creating additional std.allocator.impl
>>> package and moving actual allocators there? Or, probably, the other way
>>> around with std.allocator.core
>>>
>>> Existing flat hierarchy does not hint about internal structure in any
>>> way.
>>
>> It's good documentation, not directories, that helps understanding
>> internal structure. There are 23 files in std/experimental/allocator,
>> which seems manageable. I think we're good as we are.
>>
>> Andrei
>
> I also think putting some of the files in folder will make things
> cleaner and easier to understand.
>
> These files:
> std.experimental.allocator.affix_allocator,
> std.experimental.allocator.allocator_list,
> std.experimental.allocator.bucketizer,
> std.experimental.allocator.fallback_allocator,
> std.experimental.allocator.free_list,
> std.experimental.allocator.free_tree,
> std.experimental.allocator.gc_allocator,
> std.experimental.allocator.bitmapped_block,
> std.experimental.allocator.kernighan_ritchie,
> std.experimental.allocator.mallocator,
> std.experimental.allocator.mmap_allocator,
> std.experimental.allocator.null_allocator,
> std.experimental.allocator.quantizer,
> std.experimental.allocator.region,
> std.experimental.allocator.segregator,
> std.experimental.allocator.stats_collector;
>
> are great candidates for a std.experimental.allocator.building_blocks
> folder.

So we have:

* 1 request to change names;
* 3 requests to wank around the directory structure;
* 0 of everything else.

Sigh.


Andrei

On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote:
> On 6/13/15 4:16 PM, ZombineDev wrote:
>> On Saturday, 13 June 2015 at 15:48:31 UTC, Andrei Alexandrescu wrote:
>>> On 6/13/15 3:14 AM, Dicebot wrote:
>>>> Andrei, have you considered creating additional std.allocator.impl
>>>> package and moving actual allocators there? Or, probably, the other way
>>>> around with std.allocator.core
>>>>
>>>> Existing flat hierarchy does not hint about internal structure in any
>>>> way.
>>>
>>> It's good documentation, not directories, that helps understanding
>>> internal structure. There are 23 files in std/experimental/allocator,
>>> which seems manageable. I think we're good as we are.
>>>
>>> Andrei
>>
>> I also think putting some of the files in folder will make things
>> cleaner and easier to understand.
>>
>> These files:
>> std.experimental.allocator.affix_allocator,
>> std.experimental.allocator.allocator_list,
>> std.experimental.allocator.bucketizer,
>> std.experimental.allocator.fallback_allocator,
>> std.experimental.allocator.free_list,
>> std.experimental.allocator.free_tree,
>> std.experimental.allocator.gc_allocator,
>> std.experimental.allocator.bitmapped_block,
>> std.experimental.allocator.kernighan_ritchie,
>> std.experimental.allocator.mallocator,
>> std.experimental.allocator.mmap_allocator,
>> std.experimental.allocator.null_allocator,
>> std.experimental.allocator.quantizer,
>> std.experimental.allocator.region,
>> std.experimental.allocator.segregator,
>> std.experimental.allocator.stats_collector;
>>
>> are great candidates for a std.experimental.allocator.building_blocks
>> folder.
>
> So we have:
>
> * 1 request to change names;
> * 3 requests to wank around the directory structure;
> * 0 of everything else.
>
> Sigh.
>
>
> Andrei

can I make a request to just get it the hell in std.experimental for 2.068?

>
> So we have:
>
> * 1 request to change names;
> * 3 requests to wank around the directory structure;
> * 0 of everything else.
>
> Sigh.
>
>
> Andrei

Yesterday I have found an error in documentation and have left a comment about it on github PR.

And I only start to read the documentation, so I think there will be plenty small things to polish.

On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote:
> So we have:
>
> * 1 request to change names;
> * 3 requests to wank around the directory structure;
> * 0 of everything else.
>
> Sigh.

That is to be expected and intended for formal Phobos review. Implementation is not of much interest - it can be fixed at any point. Most important thing is to ensure that API feels right, documentation feels clear and people are in general comfortable with using proposed modules as they are.

I will do more in-depth review but it will _all_ be about API and docs and naming.

On 2015-06-14 02:24, Andrei Alexandrescu wrote:

> * 1 request to change names;
> * 3 requests to wank around the directory structure;
> * 0 of everything else.
>
> Sigh.

These are things that are easy to see right away, without looking deep inside the code.

You can interpret this as:

1. Either your implementation is already good enough

2. Or people haven't started to look at the implementation yet and are commenting on the things that they is right away

Count me on the request to change the directory structure :)

-- 
/Jacob Carlborg

On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote:
> So we have:
>
> * 1 request to change names;
> * 3 requests to wank around the directory structure;
> * 0 of everything else.
>
> Sigh.
>
>
> Andrei

How about this:

AllocatorList.allocate() calls AllocatorList.owns(), but only if you don't compile with -release. Check the assert statement. AllocatorList.owns() is non-const, so the for loops of these two functions start rearranging the root pointers and end up in an infinite loop in debug builds.

On 6/14/15 4:00 AM, Brian Schott wrote:
> On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote:
>> So we have:
>>
>> * 1 request to change names;
>> * 3 requests to wank around the directory structure;
>> * 0 of everything else.
>>
>> Sigh.
>>
>>
>> Andrei
>
> How about this:
>
> AllocatorList.allocate() calls AllocatorList.owns(), but only if you
> don't compile with -release. Check the assert statement.
> AllocatorList.owns() is non-const, so the for loops of these two
> functions start rearranging the root pointers and end up in an infinite
> loop in debug builds.

Interesting. Do you have a repro for this? Thanks! -- Andrei

On 6/13/15 11:49 PM, Dicebot wrote:
> On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote:
>> So we have:
>>
>> * 1 request to change names;
>> * 3 requests to wank around the directory structure;
>> * 0 of everything else.
>>
>> Sigh.
>
> That is to be expected and intended for formal Phobos review.
> Implementation is not of much interest - it can be fixed at any point.
> Most important thing is to ensure that API feels right, documentation
> feels clear and people are in general comfortable with using proposed
> modules as they are.
>
> I will do more in-depth review but it will _all_ be about API and docs
> and naming.

Suggestions for better names are welcome as addenda, and I will act on some, but they're no substitute for competent reviews.

We need as a community to learn how to do good reviews. Anyone can put a finger on a name of a thing and say they like another name better. ANYONE. Real review of a library is figuring out how well the proposed library's abstractions fulfill its charter and intended use cases.


Andrei

On Sunday, 14 June 2015 at 14:30:00 UTC, Andrei Alexandrescu wrote:
> Interesting. Do you have a repro for this? Thanks! -- Andrei

Not yet, but while looking for one I found that this code triggers an assertion in Region.expand()
```
module test;

import std.experimental.allocator;
import std.stdio;

private alias AllocatorType = CAllocatorImpl!(AllocatorList!(
	n => Region!Mallocator(1024 * 4), NullAllocator));

struct Big {
	ubyte[1024] bytes;
}

void main(string[] args){
	auto a = new AllocatorType;
	foreach (i; 0 .. 100) {
		a.make!Big();
		writeln(i);
	}
}
```

On Sunday, 14 June 2015 at 22:47:59 UTC, Brian Schott wrote:
> Not yet, but while looking for one I found that this code triggers an assertion in Region.expand()

Running the above code with "GCAllocator" substituted for "NullAllocator" causes an InvalidMemoryOperationError after all 100 iterations of the loop have completed.