Skip to content

eckit::utils: safe casts (1) corrected handling of converting to/from… - #334

Open
pmaciel wants to merge 2 commits into
developfrom
feature/safer-casts
Open

eckit::utils: safe casts (1) corrected handling of converting to/from…#334
pmaciel wants to merge 2 commits into
developfrom
feature/safer-casts

Conversation

@pmaciel

@pmaciel pmaciel commented Sep 4, 2026

Copy link
Copy Markdown
Member

Description

eckit::utils: safe casts:

  1. corrected handling of converting to/from unsigned/signed, and
  2. checking lower bounds

Contributor Declaration

By opening this pull request, I affirm the following:

  • All authors agree to the Contributor License Agreement.
  • The code follows the project's coding standards.
  • I have performed self-review and added comments where needed.
  • I have added or updated tests to verify that my changes are effective and functional.
  • I have run all existing tests and confirmed they pass.

🌦️ >> Documentation << 🌦️
https://sites.ecmwf.int/docs/dev-section/eckit/pull-requests/PR-334

@pmaciel

pmaciel commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Disclaimer: this is an almost completely AI-generated improvement, with testing thoroughly "manually" checked

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Size-based range checks permit value-losing conversions to bool and are not portable to padded integer representations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Improves safe integral casts by validating signedness and numeric bounds.

Changes:

  • Adds generic checked into<T> conversion.
  • Consolidates signed/unsigned helpers.
  • Expands boundary and exhaustive tests.
File summaries
File Description
src/eckit/utils/SafeCasts.h Implements checked integral conversions.
tests/utils/test_safe_casts.cc Adds boundary, constexpr, and exhaustive tests.
Review details

Suppressed comments (3)

src/eckit/utils/SafeCasts.h:59

  • This second size-based fast path has the same value-loss bug for signed inputs: into<bool>(int8_t{2}) is accepted because both types occupy one byte, then converts 2 to true. Base the fast path on representable value bits, or disallow a bool target.
        if constexpr (sizeof(T) >= sizeof(S)) {
            return true;

src/eckit/utils/SafeCasts.h:114

  • This overload also accepts signed inputs and returns them unchanged, so describing it only as unsigned-to-signed is now inaccurate. The throw condition should likewise be expressed as non-representability because signed inputs cannot exceed their corresponding signed type.
 *  Casts unsigned integer into signed.
 *  @param value to cast from.
 *  @return value cast into, same as before but signed type.
 *  @throws BadCast if used with a value > 2^(bits-1)-1

src/eckit/utils/SafeCasts.h:69

  • A strictly larger object can still have padding bits, so sizeof(T) > sizeof(S) does not guarantee that signed T represents every value of unsigned S. Use the number of value bits to make this safe-cast guarantee portable.
        // unsigned into signed: the minimum (0) is always representable, a strictly wider target always fits
        if constexpr (sizeof(T) > sizeof(S)) {
            return true;
  • Files reviewed: 2/2 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/eckit/utils/SafeCasts.h Outdated
[[nodiscard]] constexpr bool fits(S value) {
if constexpr (std::is_signed_v<S> == std::is_signed_v<T>) {
// same signedness: a target at least as wide represents every value exactly
if constexpr (sizeof(T) >= sizeof(S)) {
Comment on lines +222 to +226
CASE("Agrees with sign and magnitude for every value of the source type") {
EXPECT((fitsMatchesOracleForEveryValue<int8_t, int16_t>()));
EXPECT((fitsMatchesOracleForEveryValue<int8_t, uint16_t>()));
EXPECT((fitsMatchesOracleForEveryValue<uint8_t, int16_t>()));
EXPECT((fitsMatchesOracleForEveryValue<uint8_t, uint16_t>()));
Comment thread src/eckit/utils/SafeCasts.h Outdated
Comment on lines +96 to +99
* Casts signed integer into unsigned.
* @param value to cast from.
* @return value cast into, same as before but unsigned type.
* @throws BadCast if used with a negative value.
@codecov-commenter

codecov-commenter commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.62%. Comparing base (b15e6f7) to head (5932e72).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #334      +/-   ##
===========================================
+ Coverage    67.59%   67.62%   +0.03%     
===========================================
  Files         1182     1182              
  Lines        61862    61945      +83     
  Branches      4675     4677       +2     
===========================================
+ Hits         41813    41893      +80     
- Misses       20049    20052       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

… unsigned/signed, and (2) checking lower bounds
… unsigned/signed, and (2) checking lower bounds
@danovaro
danovaro force-pushed the feature/safer-casts branch from ddd063a to 5932e72 Compare September 8, 2026 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants