WIP fix some warnings on windows + fix 32 bits builds an incorrect use of size_t #7425
Conversation
| @@ -233,7 +233,7 @@ class bf_ref; | |||
| class bf_iterator { | |||
| uint64_t* data_area; | |||
| uint64_t* first_word_ptr; | |||
| size_t field_position; | |||
| uint64_t field_position; | |||
There was a problem hiding this comment.
this is just a slowdown on 32 bit machines. The position is never too large for a 32 bit value.
| @@ -303,11 +303,12 @@ class bf_iterator { | |||
| auto first_word = first_word_ptr[0]; | |||
| uint64_t mask = 0 - 1ULL; | |||
| if (field_size < 64) { | |||
| mask = static_cast<size_t>((1ULL << field_size) - 1); | |||
| //we should never cast to size_t, but to uint64_t in order to support 32 bits | |||
There was a problem hiding this comment.
We should not have a cast here at all.
There was a problem hiding this comment.
fixed in other PR I hope
| value &= mask; | ||
| } | ||
| // zero out field in first word: | ||
| auto first_word_mask = ~(mask << in_word_position); | ||
| uint64_t first_word_mask = ~(mask << in_word_position); |
There was a problem hiding this comment.
this is fine. (but makes no difference).
| @@ -391,14 +392,14 @@ inline void write_bitfield(uint64_t* data_area, size_t field_position, size_t wi | |||
|
|
|||
| inline int64_t sign_extend_field_by_mask(size_t sign_mask, uint64_t value) | |||
| { | |||
| uint64_t sign_extension = 0 - (value & sign_mask); | |||
| uint64_t sign_extension = (uint64_t)0 - (value & sign_mask); | |||
| @@ -174,13 +174,15 @@ class NodeHeader { | |||
|
|
|||
| static size_t unsigned_to_num_bits(uint64_t value) | |||
| { | |||
| return 1 + log2(static_cast<size_t>(value)); | |||
| return 1 + static_cast<size_t>(std::log2(value)); | |||
There was a problem hiding this comment.
Yeah, this could be a fix.
|
This PR #7427 contains the same fixes expressed in better way and less fuzzy.. |
| const auto search_vector = populate(static_cast<int>(width), value); | ||
| const auto field_count = num_fields_for_width(static_cast<int>(width)); | ||
| const auto bit_count_pr_iteration = num_bits_for_width(static_cast<int>(width)); | ||
| int64_t total_bit_count_left = static_cast<int64_t>((end - start) * width); |
There was a problem hiding this comment.
it would also be ok to use size_t for this ... perhaps that would allow for dropping a cast below?
There was a problem hiding this comment.
I think total_bit_count_left can become negative since we are taking out the amount of bits read per iteration. I'll address all the casts in a separate PR.. let's merge #7427
| @@ -1285,6 +1285,20 @@ NONCONCURRENT_TEST(Test_basic_find_GT_value_greater_32bit) | |||
|
|
|||
| #endif | |||
|
|
|||
| ONLY(Test_ArrayInt_negative_nums) | |||
There was a problem hiding this comment.
Do we want to merge an ONLY test?
What, How & Why?
WIP. Contains some fixes for some warnings + uses
uint64_tinstead ofsize_tin order to make compressed arrays compatible with 32 bits builds.☑️ ToDos
bindgen/spec.yml, if public C++ API changed