Subject: Re: [boost] [histogram] review part 1 (documentation)
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-09-18 20:54:21
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
> 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk