ARROW-1255: [Plasma] Fix typo in plasma protocol; add DCHECK for ReadXXX in plasma protocol. #887
Conversation
|
@Yeolar Looks good to me! Can you also fix the remaining Travis failure please? |
|
|
||
| Status ReadCreateRequest(uint8_t* data, ObjectID* object_id, int64_t* data_size, | ||
| int64_t* metadata_size) { | ||
| Status ReadCreateRequest(uint8_t* data, size_t size, ObjectID* object_id, |
There was a problem hiding this comment.
You probably want int64_t here for the size
There was a problem hiding this comment.
Why use int64_t for size type? The ReadXXX was called with parameter from std::vector<>, then the size is size_t type correspondingly. I see mostly using int64_t for size in the project, the reason is?
There was a problem hiding this comment.
Sorry, I thought these were arrow::Buffer objects. @pcmoritz have you considered moving all your heap allocations to an Arrow memory pool?
We prefer to use signed integer types unless we are dealing with system calls (like malloc) or the STL, basically following Google's guidance on this https://google.github.io/styleguide/cppguide.html#Integer_Types
I think it's fine to leave this as is and we can refactor later.
| using arrow::Status; | ||
|
|
||
| template <class T> | ||
| bool verify_flatbuffer(T* object, uint8_t* data, size_t size) { |
There was a problem hiding this comment.
If you receive int64_t size here, then you only have to static cast to size_t in one place when you create the verifier
There was a problem hiding this comment.
emm, it seems int64_t is used for size type in the project, I'll have a look.
|
There are some linting fixes left to be done, you can get a proposal for fixes by running 'make format' as described in https://github.com/apache/arrow/blob/1a72acdf4daa68fe6e7639acde08a0abf795a3fc/cpp/README.md I'd propose we merge this with size_t and move the heap allocations to an Arrow memory pool as a followup PR, I created a JIRA here: https://issues.apache.org/jira/browse/ARROW-1266 |
wesm
left a comment
wesm
left a comment
There was a problem hiding this comment.
+1. Thank you for the contribution!
3 participants
Related to #878, add DCHECK for ReadXXX.