-
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
Conversation
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@@ -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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind changing the underlying type of dataSize
, but I don't really have any context on the original choice.
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.
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:
- this change, which makes sense in the context of returning a string_view
- changing dataSize() et al to return size_t, which makes sense generally IMO
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
uint64_t
seems to be used extensively here to represent a number of bytes. dataSize()
is also called in a number of locations. Just to clarify, is the proposal to switch size_t
for all of these cases?
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.
Yes size_t seems appropriate for number of bytes.
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.
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 absl::string_view::size_type
has with respect to size_t
.
If folks feel it's redundant though now, I can remove 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.
this is OK with me but I'd like @mattklein123 to be the final approver.
@@ -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 comment
The 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:
- this change, which makes sense in the context of returning a string_view
- changing dataSize() et al to return size_t, which makes sense generally IMO
I slightly prefer the latter but I'm OK with this. @mattklein123 what do you prefer?
Signed-off-by: Mike Schore <mike.schore@gmail.com>
const uint64_t size = src.size(); | ||
MemBlockBuilder<uint8_t> storage(size); | ||
const size_t size = src.size(); | ||
MemBlockBuilder<uint8_t> storage(size); // Note: MemBlockBuilder takes uint64_t. |
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 use size_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.
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
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.
basically looks good, modulo a naming issue you need to fix to pass CI. Also merge master.
const uint64_t size = src.size(); | ||
MemBlockBuilder<uint8_t> storage(size); | ||
const size_t size = src.size(); | ||
MemBlockBuilder<uint8_t> storage(size); // Note: MemBlockBuilder takes uint64_t. |
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.
@@ -128,8 +127,7 @@ SymbolVec SymbolTableImpl::Encoding::decodeSymbols(const SymbolTable::Storage ar | |||
} | |||
|
|||
void SymbolTableImpl::Encoding::decodeTokens( | |||
const SymbolTable::Storage array, uint64_t size, | |||
const std::function<void(Symbol)>& symbolTokenFn, | |||
const SymbolTable::Storage array, size_t size, const std::function<void(Symbol)>& symbolTokenFn, |
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.
not your fault, but clang-tidy is dinging you for a pre-existing naming mistake here.
s/symbolTokenFn/symbol_token_fn/ here and in the header. Sorry about that.
That will help you pass CI.
/wait |
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
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.
@envoyproxy/senior-maintainers
@ggreenway do you mind doing a last pass 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.
LGTM
Description: Pulls in multiple fixes committed to upstream Envoy. - Update for resolution to TLSContext crash: envoyproxy/envoy#10030 - Fixes for 32 bit archs: envoyproxy/envoy#11726 - Fix for missing posix call on Android: envoyproxy/envoy#12081 - Additional zlib stats: envoyproxy/envoy#11782 Signed-off-by: Mike Schore <mike.schore@gmail.com>
Fixes an implicit cast error building for 32-bit Android here. Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>
Description: Pulls in multiple fixes committed to upstream Envoy. - Update for resolution to TLSContext crash: #10030 - Fixes for 32 bit archs: #11726 - Fix for missing posix call on Android: #12081 - Additional zlib stats: #11782 Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: Pulls in multiple fixes committed to upstream Envoy. - Update for resolution to TLSContext crash: #10030 - Fixes for 32 bit archs: #11726 - Fix for missing posix call on Android: #12081 - Additional zlib stats: #11782 Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Commit Message: build: minor fix for 32-bit archs
Additional Description: Fixes an implicit cast error building for 32-bit Android here.
Risk Level: Low
Testing: Builds locally
Signed-off-by: Mike Schore mike.schore@gmail.com