You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "alperak (via GitHub)" <gi...@apache.org> on 2025/07/16 15:23:27 UTC
[PR] Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds [avro]
alperak opened a new pull request, #3433:
URL: https://github.com/apache/avro/pull/3433
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 the offset and position are checked for negativity 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds [avro]
Posted by "alperak (via GitHub)" <gi...@apache.org>.
alperak commented on PR #3433:
URL: https://github.com/apache/avro/pull/3433#issuecomment-3079554872
> Does Avro itself enable `-Wuseless-cast` or does that come from some surrounding build system?
>
> 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.
`-Wuseless-cast` is enabled by Avro itself in its [CMakeLists.txt](https://github.com/apache/avro/blob/main/lang/c%2B%2B/CMakeLists.txt#L72).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #3433:
URL: https://github.com/apache/avro/pull/3433#discussion_r2454856011
##########
lang/c++/impl/FileStream.cc:
##########
@@ -205,8 +205,15 @@ class BufferCopyInInputStream : public SeekableInputStream {
void seek(int64_t position) final {
// BufferCopyIn::seek is relative to byteCount_, whereas position is
// absolute.
- in_->seek(position - byteCount_ - available_);
- byteCount_ = position;
+ 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));
Review Comment:
Do we care about 32-bit systems ?
The cast from `int64_t` to `size_t` may truncate on 32bit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #3433:
URL: https://github.com/apache/avro/pull/3433#discussion_r2454861658
##########
lang/c++/impl/FileStream.cc:
##########
@@ -205,8 +205,15 @@ class BufferCopyInInputStream : public SeekableInputStream {
void seek(int64_t position) final {
// BufferCopyIn::seek is relative to byteCount_, whereas position is
// absolute.
- in_->seek(position - byteCount_ - available_);
- byteCount_ = position;
+ 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");
+ }
+ byteCount_ = static_cast<size_t>(position);
Review Comment:
Same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #3433:
URL: https://github.com/apache/avro/pull/3433#discussion_r2454856011
##########
lang/c++/impl/FileStream.cc:
##########
@@ -205,8 +205,15 @@ class BufferCopyInInputStream : public SeekableInputStream {
void seek(int64_t position) final {
// BufferCopyIn::seek is relative to byteCount_, whereas position is
// absolute.
- in_->seek(position - byteCount_ - available_);
- byteCount_ = position;
+ 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));
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #3433:
URL: https://github.com/apache/avro/pull/3433#discussion_r2454873556
##########
lang/c++/impl/parsing/Symbol.cc:
##########
@@ -88,7 +88,12 @@ Symbol Symbol::enumAdjustSymbol(const NodePtr &writer, const NodePtr &reader) {
adj.push_back(static_cast<int>(-pos));
err.push_back(s);
} else {
- adj.push_back(static_cast<int>(it - rs.begin()));
+ auto index = it - rs.begin();
+ if constexpr (std::is_same_v<decltype(index), int>) {
Review Comment:
Need to import `<type_traits>` for `is_same_v` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds [avro]
Posted by "KalleOlaviNiemitalo (via GitHub)" <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #3433:
URL: https://github.com/apache/avro/pull/3433#issuecomment-3079503706
Does Avro itself enable `-Wuseless-cast` or does that come from some surrounding build system?
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds [avro]
Posted by "kraj (via GitHub)" <gi...@apache.org>.
kraj commented on PR #3433:
URL: https://github.com/apache/avro/pull/3433#issuecomment-3094707200
`useless-cast` warning is gcc specific, if one is using clang then this will be flagged as `unknown-warning-option`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3433:
URL: https://github.com/apache/avro/pull/3433#discussion_r2454848842
##########
lang/c++/impl/FileStream.cc:
##########
@@ -205,8 +205,15 @@ class BufferCopyInInputStream : public SeekableInputStream {
void seek(int64_t position) final {
// BufferCopyIn::seek is relative to byteCount_, whereas position is
// absolute.
- in_->seek(position - byteCount_ - available_);
- byteCount_ = position;
+ 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");
+ }
Review 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.
##########
lang/c++/impl/FileStream.cc:
##########
@@ -205,8 +205,15 @@ class BufferCopyInInputStream : public SeekableInputStream {
void seek(int64_t position) final {
// BufferCopyIn::seek is relative to byteCount_, whereas position is
// absolute.
- in_->seek(position - byteCount_ - available_);
- byteCount_ = position;
+ 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");
+ }
Review 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.
```suggestion
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));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org