[WIP] pmr container adopt #110
Conversation
fairmq/MemoryResourceTools.h
Outdated
| /// Return a vector of const ElemT, takes ownership of the message | ||
| template<typename ElemT> | ||
| std::vector<const ElemT, OwningMessageSpectatorAllocator<const ElemT>> | ||
| getVector(size_t nelem, FairMQMessagePtr message) |
There was a problem hiding this comment.
OK, so this is the intended interface. I'd vote strongly against inclusion of it because:
- The return type name is very long and I expect nobody to be able to spell it out.
autocan help, but not so much when you want to store it as a data member. - Users still see
std::vectorin the type and expect it to behave like a vector. I.e.push_back,reserve, and other operations that reallocate must work correctly. IIUC that's not the case. - A simple new type with
begin,end,operator[], andsize()that takes ownership of themessageand provides an STL interface to read-only access seems easier to create than the current solution (beginandendcould simply return a pointer, no need for a full-blown iterator type) and involves no UB.
There was a problem hiding this comment.
yes, I agree that custom containers that would allow direct adoption would be ideal, standard containers are just not designed to do this. One could try to make it work better (I think it would be possible to get the full expected functionality), but given the fact that gcc8 does not like this (libstdc++ static_asserts that T is not cv) this specific implementation has to go (or grow) anyway. I don't quite understand the need for this assertion as some functionality simply works with const elements, but they decided to start enforcing the requirement for T to be erasable, maybe for consistency.
I still think this is a useful exercise as I like the idea of containers that take ownership of the message together with the data so no additional book keeping is needed along the data path. I'll definitely give it more thought :)
- about the long type names - if we are to use allocators, the type names will be long anyway:)
There was a problem hiding this comment.
I don't quite understand the need for this assertion as some functionality simply works with const elements
You already answered your question. ;-)
if we are to use allocators, the type names will be long anyway
Unless we start using std::pmr::vector consistently.
Codecov Report
@@ Coverage Diff @@
## dev #110 +/- ##
=========================================
- Coverage 75.15% 71.9% -3.25%
=========================================
Files 66 71 +5
Lines 3916 4051 +135
=========================================
- Hits 2943 2913 -30
- Misses 973 1138 +165
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## dev #110 +/- ##
=========================================
- Coverage 75.15% 71.9% -3.25%
=========================================
Files 66 71 +5
Lines 3916 4051 +135
=========================================
- Hits 2943 2913 -30
- Misses 973 1138 +165
|
|
here's another go at this still trying to use std::vector - now the adopted vector is not const and fully functional, but still with a special allocator. |
Thx, will have a look at it soon. Also found this library: https://github.com/foonathan/array |
|
So far, I am not convinced that buffer adoption with |
this is to keep the discussion on container adoption open, contains the full code form the original pr #93