Skip to content

Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds #3433

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.

Sign up for GitHub

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

Open
alperak wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Symbol.cc and FileStream.cc: Fix -Wconversion and -Wuseless-cast on 32-bit builds #3433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions lang/c++/impl/FileStream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

@martin-g martin-g Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
}
Comment on lines +208 to +215
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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));

Copilot uses AI. Check for mistakes.
Comment on lines +213 to +215
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
byteCount_ = static_cast<size_t>(position);
Copy link
Member

@martin-g martin-g Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

available_ = 0;
}

Expand Down
7 changes: 6 additions & 1 deletion lang/c++/impl/parsing/Symbol.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>) {
Copy link
Member

@martin-g martin-g Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to import <type_traits> for is_same_v ?

adj.push_back(index); // 32-bit: already int
} else {
adj.push_back(static_cast<int>(index)); // 64-bit: long to int
}
}
}
return Symbol(Kind::EnumAdjust, make_pair(adj, err));
Expand Down