-
Notifications
You must be signed in to change notification settings - Fork 11
Refactor CodesContent with new CodesHandle #128
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.
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #128 +/- ##
===========================================
- Coverage 71.07% 70.52% -0.56%
===========================================
Files 130 132 +2
Lines 8160 8214 +54
Branches 781 783 +2
===========================================
- Hits 5800 5793 -7
- Misses 2360 2421 +61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
330cf6c to
4329de1
Compare
src/metkit/codes/CodesSplitter.cc
Outdated
|
|
||
| return eckit::message::Message(new MallocCodesContent(data, size, 0)); | ||
| return eckit::message::Message( | ||
| new CodesContent(codesHandleFromMessageCopy({static_cast<const uint8_t*>(data.get()), size}))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to run this past @simondsmart because we might impact performance due to the additional copy.
86f2a5a to
9eb32f2
Compare
9eb32f2 to
a70318a
Compare
a70318a to
5e17fd3
Compare
a08adb1 to
2240056
Compare
769c854 to
ee986b0
Compare
Description
This PR creates a copy of CodesContent with the name CodesDataContent.
CodesDataContent is using the new CodesHandle API wrapper to avoid using eccodes directly.
MallocCodesContenthas been removed as it had wrong behaviour - after a modification to the handle in returned the old data array.codesHandleFromFilehas been extended with an optional offset parameterDepending downstream dependencies arefdb- a separate PR (ecmwf/fdb#198) - unfortunately we can not test the CI using both.Contributor Declaration
By opening this pull request, I affirm the following: