Formal Review of std.range.ndslice

[Ilya Yaroshenko via Digitalmars-d]

On Thursday, 3 December 2015 at 15:07:55 UTC, Andrei Alexandrescu 
wrote:
> 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."

I will create review with professional translator at least.

> * 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.

Plans to add one example for image processing and one for 
computer vision.

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

Yes

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

will fix

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

Looks like you have read old docs. I have added a lot features 
and split module into package. LOC growed from 1.5K to 4.5K

Quick brief of changes can be found in 
https://github.com/D-Programming-Language/phobos/pull/3397 .

The latest docs (right now): 
http://dtest.thecybershadow.net/artifact/website-8937f229ab6d7bffa1c5996673347d0071563ef1-44ccae0e926aef9268d7289ec985a497/web/phobos-prerelease/std_experimental_ndslice.html

ALLOCATORS:

1. There is no any kind off allocation (string concatenations in 
asserts still needs to be checked) in updated 
std.experimental.ndslice package except the `ReshapeException` in 
the `reshape` function.

2. There is tiny example for `sliced` with `makeSlice` function, 
which user can copy-past to work with allocators.

3. `ndarray` is an example too. To make it work with allocators 
we need std.array.array to work with allocators first.

> * 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.

OK, `*ed` will be removed.

Simplest 1D Example for reverse:

data: 0 1 2 3 4 5
       ^
       ptr, stride = 1

after reverse:
data: 0 1 2 3 4 5
                 ^
                 ptr, stride = -1

No functions change data in `ndslice` package.

In updated docs there is two kind of functions:

1. std.experimental.ndslice.iteration contains functions like 
`transposed` and other `*ed` stuff (`*ed`  will be removed).
    1. This functions do _not_ change data
    2. Have the _same_ return type

2. std.experimental.ndslice.selection contains functions like 
`reshape, `blocks` and `byElement`.
    1. This functions do _not_ change data
    2. Have _another_ return type

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

Fixed to Random Access Range with slicing.
Please use the link above to access updated docs. And PR to 
access the latest docs via CyberShadow doc engine.

> * 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.

Partially fixed, still waiting for my translator

> * 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.

`ReplaceArrayWithPointer` flag was added. By default it is 
`true`. It can be used for CTFE-able code. Performance difference 
between pointer and array is very signifiant:

Binary structure with pointer:
   lengths
   strides
   ptr

Binary structure with array:
   lengths
   strides
   array (pointer + length)
   shift (shift to the front element in a slice)

It is impossible to have flexible engine for arrays/ranges 
without `shift`. Pointers have not such constraints.

> IMPLEMENTATION
>
[...]
> Very nice work. Thanks!
>
>
> Andrei

Thank you for review!

Ilya