eclipse-iceoryx/iceoryx

Can't directly assign `const` underlying value to `iox::optional`

Closed this issue · 2 comments

tdouglas-latai commented

Required information

Operating system:
Ubuntu 20.04.6 LTS

Compiler version:
Clang 11, GCC 9.4.0

Eclipse iceoryx version:
master

Observed result or behaviour:
In the implementation of iox::optional::operator= for direct assignment of the underlying value, the newValue parameter is forwarded as T instead of U, which results in a build error when attempting to assign a const value. This can be fixed by changing the two instances of std::forward<T> to std::forward<U>.

iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/optional.inl

Lines 164 to 179 in 1e17b23

// NOLINTNEXTLINE(cppcoreguidelines-c-copy-assignment-signature) justification in header
template <typename T>
template <typename U>
inline typename std::enable_if<!std::is_same<U, optional<T>&>::value, optional<T>>::type&
optional<T>::operator=(U&& newValue) noexcept
{
if (m_hasValue)
{
value() = std::forward<T>(newValue);
}
else
{
construct_value(std::forward<T>(newValue));
}
return *this;
}

Expected result or behaviour:
No build error.

Conditions where it occurred / Performed steps:

I was able to replicate the error in the test_vocabulary_optional.cpp test by adding the following cases:

TEST_F(Optional_test, DirectCopyAssignmentWithNoValue)
{
    ::testing::Test::RecordProperty("TEST_ID", "8dddd1c5-e59b-4f3c-9e6c-6fa9ac1daa86");
    iox::optional<TestClass> sut;
    const TestClass value{4711, 1337};

    sut = value;
    ASSERT_THAT(sut.has_value(), Eq(true));
    EXPECT_THAT(sut->value, Eq(4711));
    EXPECT_THAT(sut->secondValue, Eq(1337));
}

TEST_F(Optional_test, DirectCopyAssignmentWithValue)
{
    ::testing::Test::RecordProperty("TEST_ID", "66fa19ab-0a08-48d3-824c-7b259e6f15b0");
    iox::optional<TestClass> sut{TestClass{7474, 33331}};
    const TestClass value{4711, 1337};

    sut = value;
    ASSERT_THAT(sut.has_value(), Eq(true));
    EXPECT_THAT(sut->value, Eq(4711));
    EXPECT_THAT(sut->secondValue, Eq(1337));
}

The error occurs when the TestCase value is const or constexpr qualified.

iceoryx_hoofs/vocabulary/include/iox/detail/optional.inl:172:34: error: binding reference of type 'std::remove_reference<{anonymous}::Optional_test::TestClass>::type&' {aka '{anonymous}::Optional_test::TestClass&'} to 'const {anonymous}::Optional_test::TestClass' discards qualifiers
  172 |         value() = std::forward<T>(newValue);
      |                   ~~~~~~~~~~~~~~~^~~~~~~~~~

iceoryx_hoofs/vocabulary/include/iox/detail/optional.inl:176:40: error: binding reference of type 'std::remove_reference<{anonymous}::Optional_test::TestClass>::type&' {aka '{anonymous}::Optional_test::TestClass&'} to 'const {anonymous}::Optional_test::TestClass' discards qualifiers
  176 |         construct_value(std::forward<T>(newValue));
      |                         ~~~~~~~~~~~~~~~^~~~~~~~~~
elfenpiff commented

@tdouglas-latai Thanks for the bug report.

Since you essentially also fixed the problem do you want to create a pull request, and we merge it, otherwise I would take it over and fix it.

tdouglas-latai commented

Sure, I can put up a PR

  • 😄1