eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.68k stars 393 forks source link

Span assert does not compile #2253

Closed pbarone-latai closed 7 months ago

pbarone-latai commented 7 months ago

Required information

Operating system: Ubuntu 20.04.6 LTS

Compiler version: Clang 11

Eclipse iceoryx version: master

Observed result or behaviour: The following assert statement does not build ... https://github.com/eclipse-iceoryx/iceoryx/blob/b5050251f17cf390652ad6c0a76fcff23296186a/iceoryx_hoofs/vocabulary/include/iox/detail/span.inl#L58-L64

TEST_CASE("iox::span issue")
{
  const std::vector<int> v = {1, 2, 3};

  // Compiles
  [[maybe_unused]] const iox::span<const int> s1(v.begin(), v.size());

  // Does not compile
  [[maybe_unused]] const iox::span<const int> s2(v.begin(), v.end());
}

Error:

In file included from external/iceoryx/iceoryx_hoofs/vocabulary/include/iox/span.hpp:419:
external/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/span.inl:63:18: error: invalid operands to binary expression ('__gnu_cxx::__normal_iterator<const int *, std::vector<int, std::allocator<int>>>' and 'nullptr_t')
    assert(begin == nullptr || end == nullptr || begin <= end);

Expected result or behaviour: Test above should compile

Conditions where it occurred / Performed steps: See above

Additional helpful information

elBoberido commented 7 months ago

It seems the span has not enough tests. I guess the check for a nullptr should only be done when the It and End are pointer types ... and event then the check is wrong. Can you create a patch?

pbarone-latai commented 7 months ago

I can definitely add a patch and some unit tests. I'm not entirely sure I understand the intent of the original assert though. My suggestion would be to change it to simply ...

assert(begin <= end); 

Is that acceptable?

elBoberido commented 7 months ago

That should be fine. I'm also not sure why there can be different types for the begin and end iterator. I know that the STL allows such things but it is not possible with our implementation. I would also remove that.

pbarone-latai commented 7 months ago

@elBoberido https://github.com/eclipse-iceoryx/iceoryx/pull/2255 is ready for review