Refactor Relocatable Pointer and Relative Pointer

[elfenpiff]

trafficstars

Brief feature description

The relocatable pointer (iceoryx_utils/internal/relocatable_pointer) implementation needs to be cleaned up:

1. [x] add noexcept, const, replace size_t with uint64_t (done in #618 except making parameters const)
2. [x] separate implementation and declaration for all classes (#618)
3. [x] one class per header file (#618)
4. [x] add subnamespace iox::rp where everything is stored (#618)
5. [ ] restructure the unit tests / add missing tests
6. [x] add doxygen documentation (#618)
7. [ ] remove includes of base_relative_ptr.hpp & base_relocatable_ptr.hpp
8. [ ] consider removing ptr_t and const_ptr_t aliases and use void* directly (see https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597604809)
9. [x] ~~add void* get() noexcept, const void* get() const noexcept, void* getBasePtr() noexcept and const void* getBasePtr() const noexcept to BaseRelativePointer~~ -> Not implemented as SharedPointer uses get() const noexcept
10. [x] ~~consider changing return type of PointerRepository::unregisterPrt() to void (see https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597609248)~~ - Note: maybe not, it might be useful to have an indication whether the deregistration was successful
11. [ ] move duplicated code into a separate method (see https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597613171)
12. [x] rename PointerRepository::registerPtr() methods if both are needed (see https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597613855) + consider returning a cxx::unique_ptr<id_t>
13. [x] check invalid id logic (see https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597617681 and https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597619467)
14. [x] ~~add const methods to RelativePointer (see https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597620985, https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597621144 and https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597621263).~~ This might not be needed, compare with e.g. std::shared_pointer https://en.cppreference.com/w/cpp/memory/shared_ptr/operator* It should work similar to STL smart pointers.
15. [ ] invalidate other move c'tor and move assignment operator of BaseRelocatablePointer (see https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597626715 and https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597628041)
16. [ ] use composition instead of inheritance (see https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r599596822 and https://github.com/eclipse-iceoryx/iceoryx/issues/605#issuecomment-806045008) - if done, consider renaming Base...Pointer to Untyped...Pointer
17. [ ] replace RelativePointer by RelocatablePointer where applicable, e.g. Semaphore and LoFFLi

[elBoberido]

I just thought about the refactoring. Currently there are two classes, BaseRelativPointer and RelativPointer. The former one is untyped and the latter one typed with a template parameter. Neither of those is trivially copyable, which requires de-structuring (e.g. with the ChunkTuple in ChunkQueuePusher) when put into a queue for shm communication. Additionally, its 16 bytes and can therefore also not be written in on clock cycle, even if it would be trivially copyable.

I would suggest to make the static functions of BaseRelativPointer free functions, since there is now the rp namespace, e.g. rp::registerRelativePointer. With this change it should be easy to merge BaseRelativPointer and RelativPointer into a single class and just use void as template parameter if an untyped relative pointer is required.

Furthermore, to make the de-structuring superfluous another struct will be introduced RelativePointerData like proposed here https://github.com/eclipse-iceoryx/iceoryx/issues/623#issuecomment-803986280 and used to fix the torn write in the UsedChunkList

    class RelativePointerData
    {
      public:
        constexpr RelativePointerData() noexcept
            : m_idAndOffset(LOGICAL_NULLPTR)
        {
            static_assert(sizeof(RelativePointerData) <= 8U, "The RelativePointerData size must not exceed 64 bit!");
            static_assert(std::is_trivially_copyable<RelativePointerData>::value,
                          "The RelativePointerData must be trivially copyable!");
        }
        constexpr RelativePointerData(uint16_t id, uint64_t offset) noexcept
            : m_idAndOffset(static_cast<uint64_t>(id) | (offset << 16U))
        {
            cxx::Ensures(offset < MAX_OFFSET && "offset must not exceed MAX:OFFSET!");
        }

        uint16_t id() const noexcept
        {
            return static_cast<uint16_t>(m_idAndOffset & MAX_ID);
        }

        uint64_t offset() const noexcept
        {
            return (m_idAndOffset >> 16) & MAX_OFFSET;
        }

        void reset() noexcept
        {
            *this = LOGICAL_NULLPTR;
        }

        bool isLogicalNullptr() const noexcept
        {
            return m_idAndOffset == LOGICAL_NULLPTR;
        }

        static constexpr uint16_t MAX_ID{std::numeric_limits<uint16_t>::max()};
        /// @note the id is 16 bit and the offset consumes the remaining 48 bits -> max offset is 2^48 - 1
        static constexpr uint64_t MAX_OFFSET{(1ULL << 48U) - 1U};
        static constexpr uint64_t LOGICAL_NULLPTR{std::numeric_limits<uint64_t>::max()};

      private:
        uint64_t m_idAndOffset{LOGICAL_NULLPTR};
    };

This would be the member of RelativPointer and could also replace the ChunkTuple.

To keep the reviewer happy, this could be done in three steps

• static functions to free functions
• introduce RelativePointerData struct
• merge the two classes

What do you think?

Edit: The proposed RelativePointerData struct is now used in the UsedChunkList

[mossmaurice]

Moved open tasks to #1745