Formal Review of std.range.ndslice

Andrei Alexandrescu via Digitalmars-d digitalmars-d at puremagic.com
Thu Dec 3 07:07:55 PST 2015


On 12/01/2015 11:03 AM, Jack Stouffer wrote:
> On Sunday, 29 November 2015 at 20:53:43 UTC, Jack Stouffer wrote:
>> On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer wrote:
>>> This is the start of the two week formal review
>>
>> Friendly reminder that the review ends tomorrow.
>
> The two week review is over.

Thanks to all who participated in this. This is not a formal review, but 
I have a few comments (some of which echo what others said). They are 
not blocking, i.e. they shouldn't be addressed before voting.

DOCUMENTATION

* A native English speaker should make a careful pass through the 
documentation, there are a few simple things that are likely to escape 
systematically to a non-native. E.g. "which are represented with the 
Slice." -> "which are represented with the Slice type."

* There should be a synopsis at the very beginning of the module with a 
short complete example illustrating what the package can be used for.

* The use of "bifacial" for template and non-template is new?

* sliced() doesn't explain what happens if the input size is not a 
multiple of the sizes.

* Do createSlice() and ndarray() work with allocators?

* Not a fan of non-imperative function names for imperative functions, 
i.e. transposed(), swapped(), everted() etc. stand out like so many sore 
thumbs. For example the description of reversed() says "ReverseS 
direction of the iteration" so I assume it's just doing it imperatively. 
In Phobos we generally use action verbs for imperative actions and "-ed" 
for lazy behavior. Would be nice to continue that.

* Why does byElement return just an input range (and not a more powerful 
one)?

* Typo: std/experemental/range/ndslice.d

* Documentation is incomplete at the bottom of 
std.experimental.range.ndslice, i.e. there are a bunch of names without 
descriptions.

* Why is typeof((new int [1000].sliced(5, 6, 7)) Slice!(3, int*) and not 
Slice!(3, int[])? That seems to suggest there's little @safety in the 
interface.

IMPLEMENTATION

* According to git grep, in phobos there are 8139 occurrences of 'if (' 
and 2451 occurrences of 'if('. We should avoid such jitters in new code. 
Please change if/while/for/foreach/etc to include a space before the 
paren (actually some do have the space, others don't). There are other 
style violations as well, e.g. no space around operators (but only 
sometimes, inconsistently) etc. For existing code it's a slow process 
but this is new code and this is the right time to make it flush with 
the overall phobos style.

* The code should use "private" appropriately.

* Code should use local imports wherever possible.

* There's some commented-out code there, shouldn't.

* Duplication easy to eliminate between isPermutation and 
isPartialPermutation, just have the first call the second and then do 
the extra work.

* I think we have something good for isReference/hasReference in std.traits.

* Unittests should use @safe wherever possible, which may prompt changes 
in the interface and implementation.

Very nice work. Thanks!


Andrei



More information about the Digitalmars-d mailing list