Adds safe signed->unsigned, unsigned->signed casts #188

Ozaq · 2025-04-02T21:24:04Z

Adds new 'into_unsigned()' and 'into_signed' type casts that throw on conversion from a unrepresentable value in the target representation.

simondsmart · 2025-04-02T21:27:40Z

Looks nice. What is the use case you have in mind?

codecov-commenter · 2025-04-02T21:30:36Z

Codecov Report

Attention: Patch coverage is 97.36842% with 2 lines in your changes missing coverage. Please review.

Project coverage is 64.69%. Comparing base (ac5300a) to head (1841cb3).

Files with missing lines	Patch %	Lines
src/eckit/utils/SafeCasts.h	75.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #188      +/-   ##
===========================================
+ Coverage    64.64%   64.69%   +0.04%     
===========================================
  Files         1105     1107       +2     
  Lines        55988    56064      +76     
  Branches      4234     4235       +1     
===========================================
+ Hits         36193    36269      +76     
  Misses       19795    19795

☔ View full report in Codecov by Sentry.
📢 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.

github-actions · 2025-04-02T22:30:58Z

Private downstream CI failed.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14230629088.

github-actions · 2025-04-02T23:11:26Z

Private downstream CI failed.
Workflow name: private-downstream-ci-hpc
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14231269229.

Ozaq · 2025-04-03T09:40:11Z

Looks nice. What is the use case you have in mind?

I am reading a signed value in Hermes (which should have been a unsigned value), and since I am receiving the value form the network I want to ensure the service is safely converting the value, or raise an error that can be handled gracefully.

wdeconinck

I think this will be very useful!

wdeconinck · 2025-04-03T11:45:47Z

Note, the unit test fails compilation for rocky-8.6.
https://github.com/ecmwf/eckit/actions/runs/14229901435/job/39878181222?pr=188#step:3:3872

/opt/actions-runner/work/_work/eckit/eckit/tests/utils/test_safe_casts.cc:20:51: error: expected ')'
     20 |     EXPECT_EQUAL(into_unsigned(int8_t{5}), uint8_t{5});
        |                                                   ^

Sometimes wrapping the argument in parentheses could work around in my experience. Worth a try:

EXPECT_EQUAL(into_unsigned(int8_t{5}), (uint8_t{5}));

pmaciel · 2025-04-03T12:31:04Z

i like this PR :-)

But I would like to raise the discussion, shouldn't this be integrated with eckit::Translator ? It serves a similar purpose, and we'de benefit from integrating this PR's more-natural interface into the more cumbersome Translator operator()(...)

Ozaq · 2025-04-03T13:38:43Z

i like this PR :-)

But I would like to raise the discussion, shouldn't this be integrated with eckit::Translator ? It serves a similar purpose, and we'de benefit from integrating this PR's more-natural interface into the more cumbersome Translator operator()(...)

I haven't even looked into translator... (or was particularly aware of its existence) Let me have a look and i'll get back to you.

simondsmart · 2025-04-04T09:11:34Z

Note that we have already added the translate() function which somewhat improves the Translator functionality.

Ozaq · 2025-04-04T20:34:38Z

i like this PR :-)

But I would like to raise the discussion, shouldn't this be integrated with eckit::Translator ? It serves a similar purpose, and we'de benefit from integrating this PR's more-natural interface into the more cumbersome Translator operator()(...)

Ok so I had some time to look into the translator. I think this should not become part of the translator. All specializations in Translator.h deal with serialization or deserialization (one of FROM/TO are std::string). While it could be integrated into translator just from looking at the interface I don't think it would make for nice semantics.

github-actions · 2025-04-04T21:49:42Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14274524976.

github-actions · 2025-04-04T21:49:42Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14274524976.

github-actions · 2025-04-04T22:08:49Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci-hpc
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14274896755.

Ozaq · 2025-04-06T17:53:47Z

Can we move on with this as it is now or do you have specific change requests?

pmaciel · 2025-04-06T17:56:25Z

Can we move on with this as it is now or do you have specific change requests?

Yes, please merge this with Translator (in the right place) -- otherwise we will start having multiple ways of doing the same thing

wdeconinck · 2025-04-06T18:23:17Z

My guess is miscommunication. I suspect @pmaciel didn't mean to replace this functionality with eckit::Translator and he doesn't mean that you should be using eckit::Translator. Rather he means to refactor eckit::Translator to make use of this new functionality, if possible.

pmaciel · 2025-04-06T18:35:56Z

Yes indeed - to me the SafeCasts file is fine, it's just Translator should use this for its existing (and possibly missing) specialisations

Ozaq · 2025-04-07T03:55:37Z

+1 thank ms for the clarification! Will do!

pmaciel

After a call with @Ozaq we agreed to make a more modern solution, while providing a easy transition.

github-actions · 2025-04-08T08:01:15Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14327896521.

github-actions · 2025-04-08T08:01:16Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14327896521.

github-actions · 2025-04-08T08:54:36Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci-hpc
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14329200187.

github-actions · 2025-04-08T15:43:02Z

Private downstream CI failed.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14337360251.

Adds new 'into_unsigned()' and 'into_signed' type casts that throw on conversion from a unrepresentable value in the target representation.

github-actions · 2025-04-09T12:41:24Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14357007979.

github-actions · 2025-04-09T12:41:25Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14357007979.

github-actions · 2025-04-09T12:41:26Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14357007979.

github-actions · 2025-04-09T13:32:10Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci-hpc
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14358319108.

github-actions · 2025-04-09T15:43:52Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14361076528.

github-actions · 2025-04-09T15:43:53Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14361076528.

github-actions · 2025-04-09T15:43:54Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14361076528.

github-actions · 2025-04-09T17:27:36Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci-hpc
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14363166865.

github-actions · 2025-04-09T18:43:38Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci-hpc
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14364535722.

github-actions · 2025-04-09T19:10:30Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14365064496.

github-actions · 2025-04-09T19:10:31Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14365064496.

github-actions · 2025-04-09T19:10:31Z

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14365064496.

Ozaq requested review from wdeconinck, pmaciel and simondsmart April 2, 2025 21:24

wdeconinck changed the title ~~Adds safe signed->unsigned, unsigne->signed casts~~ Adds safe signed->unsigned, unsigned->signed casts Apr 3, 2025

wdeconinck approved these changes Apr 3, 2025

View reviewed changes

Ozaq force-pushed the feature/safe-casts branch from e2cb32a to ae4c9eb Compare April 4, 2025 20:15

Ozaq force-pushed the feature/safe-casts branch from ae4c9eb to 4bdfd93 Compare April 4, 2025 20:34

pmaciel approved these changes Apr 8, 2025

View reviewed changes

pmaciel force-pushed the feature/safe-casts branch from 4bdfd93 to 808aa73 Compare April 8, 2025 07:31

Adds safe signed->unsigned, unsigne->signed casts

1841cb3

Adds new 'into_unsigned()' and 'into_signed' type casts that throw on conversion from a unrepresentable value in the target representation.

Ozaq force-pushed the feature/safe-casts branch from 808aa73 to 1841cb3 Compare April 9, 2025 11:49

pmaciel merged commit 79796d6 into develop Apr 9, 2025
237 of 243 checks passed

pmaciel deleted the feature/safe-casts branch April 9, 2025 19:42

Adds safe signed->unsigned, unsigned->signed casts #188

Adds safe signed->unsigned, unsigned->signed casts #188

Conversation

Ozaq commented Apr 2, 2025

simondsmart commented Apr 2, 2025

codecov-commenter commented Apr 2, 2025 • edited Loading

Codecov Report

github-actions bot commented Apr 2, 2025

github-actions bot commented Apr 2, 2025

Ozaq commented Apr 3, 2025

wdeconinck left a comment

Choose a reason for hiding this comment

wdeconinck commented Apr 3, 2025 • edited Loading

pmaciel commented Apr 3, 2025

Ozaq commented Apr 3, 2025

simondsmart commented Apr 4, 2025

Ozaq commented Apr 4, 2025

github-actions bot commented Apr 4, 2025

github-actions bot commented Apr 4, 2025

github-actions bot commented Apr 4, 2025

Ozaq commented Apr 6, 2025

pmaciel commented Apr 6, 2025

wdeconinck commented Apr 6, 2025

pmaciel commented Apr 6, 2025

Ozaq commented Apr 7, 2025

pmaciel left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 8, 2025

github-actions bot commented Apr 8, 2025

github-actions bot commented Apr 8, 2025

github-actions bot commented Apr 8, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

codecov-commenter commented Apr 2, 2025 •

edited

Loading

wdeconinck commented Apr 3, 2025 •

edited

Loading