Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Meta-programming tools and arrow types traits support. #49
Meta-programming tools and arrow types traits support. #49
Changes from 18 commits
bee6170
910bfa1
30bc8ec
ec61b04
fc27ad0
e043ef8
c1e2f7e
9e2f3c0
73966ff
1ce1c78
4b7d2db
e6d52b9
67b4769
f98b986
1bd7f2b
2efac9c
e3e53bb
6125e1c
5dccd5c
ca28676
f139fab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that we have used
uint8_t
overstd::byte
for the buffer.Is
uint8_t
use for interoperability with C, or are there other reason not to usestd::byte
?Also, if we use
uint8_t
we must:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main "classic" issue is that because
std::byte
has a strong type, not implicitly converted to other similar types, it's not automatically compatible with functions working withchar
or char-like types, in particular when interfacing with C. Lots of explicit casts ensures. Not a big problem if you like correctness but it does obfuscate the code. It's the main criticism I'm aware of.In this library, I'm not sure what would be best, but the C api kinds of forces us with at least some of the types?
hmm actually shouldnt we always check that? I'll add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think, @JohanMabille?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C Api uses
void*
for the buffers, so we'll have to cast anyway. I would actually be in favor of replacing theuint8_t
withbyte
in the array_data buffers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I went with an alias
byte_t
so that we can change that easilly, and addedvector<byte_t>
and I suppose we'll see how it goes from here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semantically, I think we should rename
arrow_traits
oris_arrow_traits
given the definition of this concept.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didnt get what you meant, but just in case, here is the reason I named them this way:
arrow_traits
is similar in purpose tostd::iterator_traits
so I followed that naming pattern;is_...
makes sense if I dont have the choice of just naming itarrow_traits
.I'm open to suggestions for alternatives, you seem to think
is_arrow_traits
doesnt match what it does or at least the documentation?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my remark was not clear.
is_arrow_traits
checks for more than a template type parameter being anarrow_traits
:is_arrow_traits
currently is defined on several condition including the one here (on line 130), but also other ones bellow (which my previous comment does not show nor mention). In this regards, I think the naming is inconsistent.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These conditions (mostly interface checks) are all requirements for a valid (as in usable by the rest of the library)
arrow_traits
specialization, so the name seems fine to me, but maybe I'm missing your point? Are you suggesting something likeis_valid_arrow_traits
, to be clearer?