Add non-template get_xxx/is_xxx methods to element#949
Conversation
a1081b1 to
0b8c357
Compare
|
@lemire the other PR is based on this one (because tests), FYI, in case you didn't see this. |
|
(As usual, no actual rush.) |
|
(I just rebased the other PR without this, not too many changes, that way progress can occur.) |
lemire
left a comment
There was a problem hiding this comment.
Obviously very nice.
I guess that there is no need to update the markdown documentation?
|
@lemire I think it could use some ... I'm just not sure what yet. The get(T) patch interacts with it a ton. |
|
@lemire I'm going to merge this and move forward, but we need to think about whether get_unsigned or get_uint64_t or something else suits us better. I haven't found a rubrick that lets me say one is clearly better than the other. I'm a little worried about uint64_t in particular because my impression is different people use different type names for the unsigned 64-bit int. |
I think we need to move to "get_uint64". I think that the "_t" is actually a mistake. It is meant to tell you that the name is a type.So "get_uint64_t" to me means the type named "get_uint64". The "_t" suffix should be reserved for types. (Sorry if I did not catch it right away. It took me some time to think it through.) |
|
@lemire I'm good with that. It's a small change we can make pre-0.4. |
This fixes #781, adding to both element and result:
I'm unsure about the naming of get_double, get_int64_t and get_uint64_t. I could see get_float, get_integer and get_unsigned as well, but went with this because then I don't have to introduce concepts.
To accomplish this, I also had to move
internal::tape_refout of the way: it has some methods with the same name, and that was causing C++ confusion. Instead of being a base class of element, internal::tape_ref is now a member. I did the same for array and object and their iterators as well, for consistency. This is nice in part because now there isn't a distracting private base class for these classes in the documentation. It's all hidden exactly as it should be.I also refactored the casting tests some so that I could easily add tests for this functionality.