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