Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds #3433
Conversation
…2-bit builds
Symbol.cc:
it - rs.begin() returns a value of type std::ptrdiff_t.
On 32-bit systems, this is typically int, so casting it to int is redundant
and triggers -Wuseless-cast.
FileStream.cc:
size_t is 32-bit on 32-bit systems, while position is int64_t.
Casting int64_t to size_t can lead to truncation, causing -Wconversion errors.
The fix ensures that the offset and position are checked to be non-negative before conversion.
Fix:
lib32-avro-c++/1.12/sources/avro-c++-1.12/lang/c++/impl/parsing/Symbol.cc:91:27:
error: useless cast to type 'int' [-Werror=useless-cast]
91 | adj.push_back(static_cast<int>(it - rs.begin()));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
lib32-avro-c++/1.12/sources/avro-c++-1.12/lang/c++/impl/FileStream.cc:208:41:
error: conversion from 'int64_t' {aka 'long long int'} to 'size_t'
{aka 'unsigned int'} may change value [-Werror=conversion]
208 | in_->seek(position - byteCount_ - available_);
| ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
lib32-avro-c++/1.12/sources/avro-c++-1.12/lang/c++/impl/FileStream.cc:209:22:
error: conversion from 'int64_t' {aka 'long long int'} to 'size_t'
{aka 'unsigned int'} may change value [-Werror=conversion]
209 | byteCount_ = position;
| ^~~~~~~~
cc1plus: all warnings being treated as errors
These changes fix build failures on 32-bit systems and ensure safe type conversions.
Signed-off-by: Alper Ak <alperyasinak1@gmail.com>
|
Does Avro itself enable I doubt that warning will reveal any actual bug, so my first inclination would be to disable it altogether. But I'm not using the Avro C++ library so my opinion shouldn't have much weight here. |
|
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes compiler warnings (-Wconversion and -Wuseless-cast) that occur when building on 32-bit systems. The changes ensure type-safe conversions between platform-dependent types.
Key Changes:
- Added compile-time type checking in Symbol.cc to avoid useless casts on 32-bit systems
- Added validation and explicit casts in FileStream.cc to prevent unsafe int64_t to size_t conversions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lang/c++/impl/parsing/Symbol.cc | Uses if constexpr to conditionally cast based on platform type |
| lang/c++/impl/FileStream.cc | Adds negative value checks and explicit casts for seek operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| int64_t offset = position - static_cast<int64_t>(byteCount_) - static_cast<int64_t>(available_); | ||
| if (offset < 0) { | ||
| throw Exception("Negative offset in seek"); | ||
| } | ||
| in_->seek(static_cast<size_t>(offset)); | ||
| if (position < 0) { | ||
| throw Exception("Negative position not allowed"); | ||
| } |
There was a problem hiding this comment.
The negative offset check occurs after the subtraction, which means if position is less than byteCount_ + available_, a negative offset will be computed and then converted to a very large size_t value when cast on line 212. The check should occur before the cast, but the cast happens inside the function call. Consider restructuring to avoid passing a potentially negative value through the cast, or ensure the arithmetic cannot produce negative results through earlier validation.
| int64_t offset = position - static_cast<int64_t>(byteCount_) - static_cast<int64_t>(available_); | |
| if (offset < 0) { | |
| throw Exception("Negative offset in seek"); | |
| } | |
| in_->seek(static_cast<size_t>(offset)); | |
| if (position < 0) { | |
| throw Exception("Negative position not allowed"); | |
| } | |
| if (position < 0) { | |
| throw Exception("Negative position not allowed"); | |
| } | |
| int64_t offset = position - static_cast<int64_t>(byteCount_) - static_cast<int64_t>(available_); | |
| if (offset < 0) { | |
| throw Exception("Negative offset in seek"); | |
| } | |
| in_->seek(static_cast<size_t>(offset)); |
| if (position < 0) { | ||
| throw Exception("Negative position not allowed"); | ||
| } |
There was a problem hiding this comment.
The negative position check should occur at the beginning of the function, before any calculations are performed with position. Moving this validation earlier would fail fast and avoid unnecessary computation.
| if (offset < 0) { | ||
| throw Exception("Negative offset in seek"); | ||
| } | ||
| in_->seek(static_cast<size_t>(offset)); |
There was a problem hiding this comment.
Do we care about 32-bit systems ?
Update: Actually this PR is specifically for improving the things on 32bit systems.
The cast from int64_t to size_t may truncate on 32bit
| if (position < 0) { | ||
| throw Exception("Negative position not allowed"); | ||
| } | ||
| byteCount_ = static_cast<size_t>(position); |
| } else { | ||
| adj.push_back(static_cast<int>(it - rs.begin())); | ||
| auto index = it - rs.begin(); | ||
| if constexpr (std::is_same_v<decltype(index), int>) { |
There was a problem hiding this comment.
Need to import <type_traits> for is_same_v ?
4 participants
Symbol.cc:
it - rs.begin() returns a value of type std::ptrdiff_t. On 32-bit systems, this is typically int, so casting it to int is redundant and triggers -Wuseless-cast.
FileStream.cc:
size_t is 32-bit on 32-bit systems, while position is int64_t. Casting int64_t to size_t can lead to truncation, causing -Wconversion errors. The fix ensures that the offset and position are checked to be non-negative before conversion.
Fix:
lib32-avro-c++/1.12/sources/avro-c++-1.12/lang/c++/impl/parsing/Symbol.cc:91:27: error: useless cast to type 'int' [-Werror=useless-cast]
91 | adj.push_back(static_cast(it - rs.begin()));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
lib32-avro-c++/1.12/sources/avro-c++-1.12/lang/c++/impl/FileStream.cc:208:41: error: conversion from 'int64_t' {aka 'long long int'} to 'size_t' {aka 'unsigned int'} may change value [-Werror=conversion]
208 | in_->seek(position - byteCount_ - available_);
| ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
lib32-avro-c++/1.12/sources/avro-c++-1.12/lang/c++/impl/FileStream.cc:209:22:
error: conversion from 'int64_t' {aka 'long long int'} to 'size_t'
{aka 'unsigned int'} may change value [-Werror=conversion]
209 | byteCount_ = position;
| ^~~~~~~~
cc1plus: all warnings being treated as errors
These changes fix build failures on 32-bit systems and ensure safe type conversions.