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