This PR was discussed in person instead of over GitHub. The main comments are listed below, with [*] checkboxes indicating whether the comment has since been resolved:
[x] The packages are missing versions
--> The CI system has now been updated to manage versions, changelogs and releases. Once this PR is ready to merge, it will be assigned a unique version.
[x] Too few comments
--> I will add more comments.
[x] Map->Stream and Stream->Map; why do some return values while others don't
--> The return values communicate success/failure. I'll update the code so they all detect more failure cases.
stream_conversions.cc
[x] :102 Implications of using .emplace(...)
--> The usages are safe :)
[x] :143 Should mention C++17 requirements
--> This is listed in the CMakeLists, and I added a todo to include it in the upcoming documentation.
[x] :180 Should this return an error/raise an exception?
--> Yes that'd be better, thanks!
[ ] :194 Why shared_ptr?
--> The standard pointer type for map data structures in wavemap is shared, as we usually share their ownership between the different components (multiple integrators, the server,...). On this line, we just initialize the deserializer's output map (passed as an argument by reference). An alternative would be to always initialize it as a unique_ptr which users can then convert to a shared_ptr if needed (the std library allows converting unique_ptrs to shared_ptrs; but not vice versa). However this might be confusing to users that are not aware this is possible. I'll think about it more.
--> It would probably be best to change the default pointer type (defined in most classes as ::Ptr) to unique_ptr, and add additional ::SharedPtr typdefs so the intent is more clearly communicated. But since this would affect many files and conflict with the other active PRs, I suggest to keep it for a separate refactoring round.
[x] :121, :226 code repetition
--> Although the way both data structures are serialized is similar, there are subtle differences spread throughout the whole flow. I initially wrote a version without code repetition using templating, but this was hard to read and I didn't see a more intuitive/simpler way. So I think it's best to leave it as two separate methods for now.
[ ] :283 continue behaves like a jump (unintuitive)
--> This is a good point. Since continue is used in this way quite often in the codebase, I suggest to address it in a separate refactoring round (when there're less other active PRs that would conflict) :)
wavemap_server.cc
[x] :78 return False missing?
--> You're right. Somehow I forgot to commit the fix to this branch.
This PR was discussed in person instead of over GitHub. The main comments are listed below, with [*] checkboxes indicating whether the comment has since been resolved:
continue
is used in this way quite often in the codebase, I suggest to address it in a separate refactoring round (when there're less other active PRs that would conflict) :)