-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
build: minor fix for 32-bit archs #11726
Changes from all commits
96bc163
2bbb403
df37ac4
f06247c
80497bc
6398bce
27b7027
6d56ecd
6dbb02f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,7 +93,7 @@ class SymbolTableImpl : public SymbolTable { | |
/** | ||
* Decodes a uint8_t array into a SymbolVec. | ||
*/ | ||
static SymbolVec decodeSymbols(const SymbolTable::Storage array, uint64_t size); | ||
static SymbolVec decodeSymbols(const SymbolTable::Storage array, size_t size); | ||
|
||
/** | ||
* Decodes a uint8_t array into a sequence of symbols and literal strings. | ||
|
@@ -103,18 +103,18 @@ class SymbolTableImpl : public SymbolTable { | |
* | ||
* @param array the StatName encoded as a uint8_t array. | ||
* @param size the size of the array in bytes. | ||
* @param symbolTokenFn a function to be called whenever a symbol is encountered in the array. | ||
* @param stringVIewTokeNFn a function to be called whenever a string literal is encountered. | ||
* @param symbol_token_fn a function to be called whenever a symbol is encountered in the array. | ||
* @param string_view_token_fn a function to be called whenever a string literal is encountered. | ||
*/ | ||
static void decodeTokens(const SymbolTable::Storage array, uint64_t size, | ||
const std::function<void(Symbol)>& symbolTokenFn, | ||
const std::function<void(absl::string_view)>& stringViewTokenFn); | ||
static void decodeTokens(const SymbolTable::Storage array, size_t size, | ||
const std::function<void(Symbol)>& symbol_token_fn, | ||
const std::function<void(absl::string_view)>& string_view_token_fn); | ||
|
||
/** | ||
* Returns the number of bytes required to represent StatName as a uint8_t | ||
* array, including the encoded size. | ||
*/ | ||
uint64_t bytesRequired() const { | ||
size_t bytesRequired() const { | ||
return data_bytes_required_ + encodingSizeBytes(data_bytes_required_); | ||
} | ||
|
||
|
@@ -130,13 +130,13 @@ class SymbolTableImpl : public SymbolTable { | |
* @param number A number to encode in a variable length byte-array. | ||
* @return The number of bytes it would take to encode the number. | ||
*/ | ||
static uint64_t encodingSizeBytes(uint64_t number); | ||
static size_t encodingSizeBytes(uint64_t number); | ||
|
||
/** | ||
* @param num_data_bytes The number of bytes in a data-block. | ||
* @return The total number of bytes required for the data-block and its encoded size. | ||
*/ | ||
static uint64_t totalSizeBytes(uint64_t num_data_bytes) { | ||
static size_t totalSizeBytes(size_t num_data_bytes) { | ||
return encodingSizeBytes(num_data_bytes) + num_data_bytes; | ||
} | ||
|
||
|
@@ -167,10 +167,10 @@ class SymbolTableImpl : public SymbolTable { | |
* @param The encoded byte array, written previously by appendEncoding. | ||
* @return A pair containing the decoded number, and the number of bytes consumed from encoding. | ||
*/ | ||
static std::pair<uint64_t, uint64_t> decodeNumber(const uint8_t* encoding); | ||
static std::pair<uint64_t, size_t> decodeNumber(const uint8_t* encoding); | ||
|
||
private: | ||
uint64_t data_bytes_required_{0}; | ||
size_t data_bytes_required_{0}; | ||
MemBlockBuilder<uint8_t> mem_block_; | ||
}; | ||
|
||
|
@@ -229,7 +229,7 @@ class SymbolTableImpl : public SymbolTable { | |
* @param size the size of the array in bytes. | ||
* @return std::string the retrieved stat name. | ||
*/ | ||
std::vector<absl::string_view> decodeStrings(const Storage array, uint64_t size) const; | ||
std::vector<absl::string_view> decodeStrings(const Storage array, size_t size) const; | ||
|
||
/** | ||
* Convenience function for encode(), symbolizing one string segment at a time. | ||
|
@@ -403,16 +403,16 @@ class StatName { | |
bool operator!=(const StatName& rhs) const { return !(*this == rhs); } | ||
|
||
/** | ||
* @return uint64_t the number of bytes in the symbol array, excluding the | ||
* overhead for the size itself. | ||
* @return size_t the number of bytes in the symbol array, excluding the | ||
* overhead for the size itself. | ||
*/ | ||
uint64_t dataSize() const; | ||
size_t dataSize() const; | ||
|
||
/** | ||
* @return uint64_t the number of bytes in the symbol array, including the | ||
* @return size_t the number of bytes in the symbol array, including the | ||
* overhead for the size itself. | ||
*/ | ||
uint64_t size() const { return SymbolTableImpl::Encoding::totalSizeBytes(dataSize()); } | ||
size_t size() const { return SymbolTableImpl::Encoding::totalSizeBytes(dataSize()); } | ||
|
||
/** | ||
* Copies the entire StatName representation into a MemBlockBuilder, including | ||
|
@@ -466,7 +466,8 @@ class StatName { | |
* hasher and comparator. | ||
*/ | ||
absl::string_view dataAsStringView() const { | ||
return {reinterpret_cast<const char*>(data()), dataSize()}; | ||
return {reinterpret_cast<const char*>(data()), | ||
static_cast<absl::string_view::size_type>(dataSize())}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't the underlying problem the abuse of uint64_t in line 409 above for dataSize()? There's a significant penalty for using a 64 bit int in 32 bit contexts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind changing the underlying type of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I'd prefer size_t for this; IIUC Envoy style has veered toward using uint64_t. @mattklein123 may have more context. I'm fine with either:
I slightly prefer the latter but I'm OK with this. @mattklein123 what do you prefer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure we can use size_t. Typically we have used explicit types for things that can wrap, but for something like this size_t seems fine. /wait There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes size_t seems appropriate for number of bytes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Despite updating the type used for representing byte counts throughout the file, I left this cast in; I don't know what sort of contract If folks feel it's redundant though now, I can remove it. |
||
} | ||
|
||
const uint8_t* size_and_data_{nullptr}; | ||
|
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.
In theory
MemBlockBuilder
could be updated to usesize_t
, too, and I suspect there may be other places we could switch over as well. My feeling is that that's a bit outside the scope of this change, however.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.
agreed; outside the scope for now.