Refactor CodesContent with new CodesHandle #128
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #128 +/- ##
===========================================
+ Coverage 70.51% 70.84% +0.32%
===========================================
Files 132 133 +1
Lines 8107 8058 -49
Branches 781 779 -2
===========================================
- Hits 5717 5709 -8
+ Misses 2390 2349 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pgeier
force-pushed
the
feature/METK-145-Refactor-CodesContent
branch
3 times, most recently
from
330cf6c to
4329de1
Compare
| using Type = std::decay_t<decltype(v)>; | ||
| if constexpr (std::is_same_v<Type, std::string> || std::is_arithmetic_v<Type>) { | ||
| gather.setValue(name, std::forward<decltype(v)>(v)); | ||
| } |
There was a problem hiding this comment.
Please emit an error message if an unexpected type is encountered.
src/metkit/codes/api/CodesAPI.h
Outdated
| /// Return the pointer to the underlying buffer | ||
| /// @return Contiguous array to the underlying buffer. | ||
| /// @see messageSize() |
There was a problem hiding this comment.
Please mention the lifetime of Span
src/metkit/codes/api/CodesAPI.h
Outdated
| virtual Span<const uint8_t> messageData() const = 0; | ||
|
|
||
| /// Retrieve binary size of the handled message. | ||
| /// @return Offset of the message in the underlying buffer. |
src/metkit/codes/CodesSplitter.cc
Outdated
| int err = 0; | ||
| void* data = wmo_read_any_from_stream_malloc(&handle_, &readcb, &size, &err); | ||
| int err = 0; | ||
| auto deleter = [&](uint8_t* ptr) { ::free(ptr); }; |
There was a problem hiding this comment.
Please use named captures and capture by value. Capturing by reference is save in this case but it leaves a bit of a refactoring trap if data is ever returned from the function you get a dangling reference from inside the lambda.
On a second look: remove the ampersand, you are not capturing anything.
|
|
||
| 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.
We need to run this past @simondsmart because we might impact performance due to the additional copy.
pgeier
force-pushed
the
feature/METK-145-Refactor-CodesContent
branch
2 times, most recently
from
7e50702 to
86f2a5a
Compare
pgeier
force-pushed
the
feature/METK-145-Refactor-CodesContent
branch
from
86f2a5a to
9eb32f2
Compare
Description
This PR is using the recently added CodesHandle in the CodesContent to avoid using eccodes headers directly.
The
MallocCodesContenthas been removed as it had wrong behaviour - after a modification to the handle in returned the old data array.Depending downstream dependencies are
fdb- 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: