Re: [boost] [histogram] review part 1 (documentation)

Subject: Re: [boost] [histogram] review part 1 (documentation)
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-09-18 20:54:21

AMDG

On 09/18/2018 02:04 PM, Hans Dembinski via Boost wrote:
> <snip lots>
>> - Is there a reason that CMakeLists.txt is in
>>   build/ instead of test/?
>>
> 
> Mateusz moved the CMakeLists.txt into the root directory which is
> better/more straight forward and follows the example of hana and compute.
> We could also move it to test as you suggest, but I prefer it in the root
> directory.
> I read in some Boost guides (I think it was on the blincubator, but I can't
> find the source just now and blincubator.com gives me a 403 error today)
> that all build scripts should reside in the build directory or next to the
> source. So I put it in "build".
> 

  That's not the correct interpretation of the guideline
(perhaps it needs to be stated more clearly as you're
not the first to make that mistake).  build/ is specifically
for the scripts that build a separately compiled library,
which you do not need.  Note that the corresponding Jamfile,
which does essentially the same thing, lives in test/ and
this is required for integration.  I would probably put
CMakeLists.txt in test/ with an (optional) root CMakeLists.txt
that just calls add_subdirectory.

>> - It doesn't seem very sane to have the value_type
>>   be a reference.  Is there any reason for this
>>   other than the fact that you specify the exact
>>   signature of axis_type::index?
>>
> 
> This is a mistake, the method is not required to accept a reference,
> pass-by-value is also possible. How should I write it to indicate that
> passing by value or reference is possible?
> 

  I would probably handle it with an "as if" clause.
That is to say, concrete implementations need
to handle every usage that is legal for pass-by-value,
but do not necessarily need to actually use pass-by-value.
(To be strictly formally correct, this may require
additional restrictions like forbidding usage via a
pointer-to-member-function, which can distinguish
the exact argument type.)

> 
>> - I would prefer to set the size of a storage_type
>>   in the constructor instead of using reset, if
>>   that's possible.
>>
> 
> I was flip-flopping on that one. A call `value.reset(n)` can be replaced by
> `value = storage_type(n)`. I don't see a good reason to prefer one over the
> other, do you?
> 

  After starting to look at the source, I think
reset, as you have it, is better, because it
allows the storage_type to carry additional
state safely (e.g. an allocator).

> 
>> - The passive voice is overused.
>>
> 
> And in the last paper that I submitted to a journal, they complained about
> my use of active voice...
> 

  I did also notice that you used the second person
quite a bit, which gives a somewhat informal,
conversational feel.  This is fine for tutorials
but may not be appropriate in all circumstances.

In Christ,
Steven Watanabe