Add more C-APIs #2641
Add more C-APIs #2641
Conversation
benquike
force-pushed
the
main
branch
3 times, most recently
from
0e7d818 to
c6035dc
Compare
Signed-off-by: Hui Peng <[email protected]>
Signed-off-by: Hui Peng <[email protected]>
xls_value_get_flat_bit_count xls_value_populate_from_bytes Signed-off-by: Hui Peng <[email protected]>
There was a problem hiding this comment.
Started with some comments and wondering if you really need the "internal looking" Populate() API or if we can do with constructor APIs which are cleaner because they conform to immutability and don't need to understand the internal bit packing and unpacking position assumptions
| // Returns false if the bit_count is less than the flat bit count of the value | ||
| // or if the value cannot be populated from the bytes, in which case the error | ||
| // message is written to error_out. error_out must be freed by the caller with | ||
| // xls_c_str_free. |
There was a problem hiding this comment.
I think this comment should be in the header not in the impl
| // xls_c_str_free. | ||
| bool xls_value_populate_from_bytes(struct xls_value* value, | ||
| const uint8_t* bytes, size_t byte_count, | ||
| size_t bit_count, char** error_out) { |
There was a problem hiding this comment.
the real question for interfaces like these is how the final partial byte at the end of the array is expected to be populated and how it corresponds to the populated BitVector's value, e.g. you could imagine MSbs of the last byte or LSbs of the last byte or padding requirements or etc -- we should make sure it's well documented in the C API documentation since it's committing to it as a public interface
There was a problem hiding this comment.
I agree that maintaining a constructor style API to retain immutability in values is a good idea.
However at the same time, it might also be very inconvenient, especially considering that value can be nested. e.g., mutating a single bit in a tree of values requires reconstructing the whole tree.
The idea to introduce this API is to make it easier in such cases.
I think the byte ordering issue you mentioned here is not unique to this specific API, it is also present in APIs like xls_bits_make_ubits. I agree that we should document it better.
| CHECK(value != nullptr); | ||
| CHECK(bytes != nullptr); | ||
| CHECK(error_out != nullptr); | ||
| auto* cpp_value = reinterpret_cast<xls::Value*>(value); |
There was a problem hiding this comment.
I think mutating XLS values in place is not great -- I think my overall question is can we prefer a constructor API or you really need/want this populate API for maximum speed?
| // convert a type to a bits type, return false if the type is not a bits | ||
| // type and set the error_out to the error message, error_out should be freed | ||
| // with xls_c_str_free; otherwise return true and set the result_out to the | ||
| // bits type, result_out *does not* need to be freed. |
There was a problem hiding this comment.
comments should all be in the .h
you may also want to note the retval is expected that it can (and currently will) alias the input type pointer
| // convert a type to a tuple type, return false if the type is not a tuple | ||
| // type and set the error_out to the error message, error_out should be freed | ||
| // with xls_c_str_free; otherwise return true and set the result_out to the | ||
| // tuple type, result_out *does not* need to be freed. |
There was a problem hiding this comment.
I think a block comment will be better than duplicating on each function since then the likelihood they get out of sync goes up, the one benefit of not having a javadoc like system
No description provided.