Uh oh!
There was an error while loading. Please reload this page.
log/internal: fix ambiguous EncodeVarint calls where int32_t is not int - #2149
Open
vasko-110 wants to merge 1 commit into
Open
log/internal: fix ambiguous EncodeVarint calls where int32_t is not int#2149vasko-110 wants to merge 1 commit into
vasko-110 wants to merge 1 commit into
Conversation
absl/log/internal/proto.h declares EncodeVarint overloads for the fixed-width types uint64_t, int64_t, uint32_t and int32_t, but not for plain int or bool. This only resolves unambiguously when int32_t is a typedef for int, which is not guaranteed: on newlib targets such as arm-none-eabi, int32_t is long int. There, absl::LogEntry::source_line() (an int) and the bool alternative of StructuredProtoField::Varint have no exact match, and conversion to unsigned int, long and long long all rank equally, so both calls are ambiguous and the build fails. Add a bool overload, which also makes the Varint bool alternative explicit rather than relying on integral promotion, and cast source_line() at the call site. An int overload cannot be added because it would redeclare the int32_t overload where int32_t is int. Also add a structured_proto_test case for the Varint bool alternative, which was declared in the variant but never exercised.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#2148
absl/log/internal/proto.hdeclaresEncodeVarintoverloads foruint64_t,int64_t,uint32_tandint32_t, but not for plainintorbool— and two call sites pass exactly those:log_message.ccpassessource_line()(anint), andstructured_proto.ccvisits theboolalternative ofStructuredProtoField::Varint. This resolves only becauseint32_tis normally a typedef forint; on newlib/arm-none-eabi, where it islong int, both calls are ambiguous and the build fails. Verbatim diagnostics and a host-reproducible reduction are in the issue.Adds a
booloverload and castssource_line()at the call site — anintoverload is not an option, it would redeclare theint32_tone whereint32_tisint. Also adds astructured_proto_testcase for theboolalternative, which the variant declares but nothing exercised.Happy to collapse the overload set into a single
std::is_integralconstrained template instead, if you prefer that shape.Testing
Both files fail to compile with
arm-none-eabi-g++ 13.2.1onmasterand compile cleanly with this change;ctest -R '^absl_log'passes 14/14 on the host. The new test does not fail without the fix whereint32_tisint— it covers a previously untested variant alternative.