Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for cxx::function_ref #1456

Merged
merged 4 commits into from
Jul 15, 2022
Merged

iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for cxx::function_ref #1456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5bffb0
iox-#1394 Address Axivion findings for cxx::function_ref
mossmaurice Jul 1, 2022
f49d5fc
iox-#1196 Fix clang-tidy warnings in cxx::function_ref
mossmaurice Jul 7, 2022
c9c54dd
iox-#1394 Fix clang-tidy warnings in function_ref tests
mossmaurice Jul 15, 2022
16fd6c2
iox-#1394 Add missing declaration of swap to .hpp
mossmaurice Jul 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions iceoryx_hoofs/include/iceoryx_hoofs/cxx/function_ref.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@
//
// SPDX-License-Identifier: Apache-2.0

// AXIVION DISABLE STYLE AutosarC++19_03-A16.2.3 : <type_traits> is included through "type_traits.hpp"

#ifndef IOX_HOOFS_CXX_FUNCTION_REF_HPP
#define IOX_HOOFS_CXX_FUNCTION_REF_HPP

// AXIVION Next Line AutosarC++19_03-A16.2.2 : Needed for Expects and Ensures macros
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the problem here? Does it not recognize that something from this header is used? (maybe because it is a macro?) ...

Copy link
Contributor Author

@mossmaurice mossmaurice Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatthiasKillat AFAIU Axivion does not detect that the macro from requires.hpp is used.

Copy link
Contributor

@MatthiasKillat MatthiasKillat Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike that tool failure results in these suppression comments.

-1 for the tool I guess

#include "iceoryx_hoofs/cxx/requires.hpp"
#include "iceoryx_hoofs/cxx/type_traits.hpp"

#include <cstddef>
#include <iostream>
#include <memory>
#include <type_traits>

namespace iox
Expand All @@ -33,6 +33,12 @@ namespace cxx
template <typename SignatureType>
class function_ref;

/// @brief Type trait which checks for the same decayed type
/// @tparam[in] T1 first type
/// @tparam[in] T2 second type
template <typename T1, typename T2>
using has_same_decayed_type = typename std::
integral_constant<bool, bool(std::is_same<typename std::decay<T1>::type, typename std::decay<T2>::type>::value)>;

/// @brief cxx::function_ref is a non-owning reference to a callable.
///
Expand Down Expand Up @@ -61,41 +67,37 @@ class function_ref;
/// callback();
/// @endcode
template <class ReturnType, class... ArgTypes>
class function_ref<ReturnType(ArgTypes...)>
class function_ref<ReturnType(ArgTypes...)> final
{
using SignatureType = ReturnType(ArgTypes...);

template <typename T1, typename T2>
using has_same_decayed_type = typename std::integral_constant<
bool,
bool(std::is_same<typename std::decay<T1>::type, typename std::decay<T2>::type>::value)>;

public:
~function_ref() noexcept = default;

function_ref(const function_ref&) noexcept = default;

function_ref& operator=(const function_ref&) noexcept = default;
function_ref& operator=(const function_ref&) & noexcept = default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add a reference here? Was it a warning? Should we do this for all copy assignments?

So this rule forbids code like:

std::move(a) = b;

Which is valid and well defined but on the first glimpse weird. But take a look at this here

template<typename T>
void initializeSomeArgs(T&& args) {
    std::forward<T>(args) = someT;
}

Here we take args as universal reference (so sometimes also as rvalue reference) and would like to assign a new value to it.
Why should we forbid that? What kind of error can possible originate from that?

Copy link
Contributor

@MatthiasKillat MatthiasKillat Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elfenpiff In the example you provided it can have outside effect, depending on the type it is instantiated with so anything can go wrong but this is no reason not to allow since this holds for any references or pointers.

While it will not be useful often (or at all) for function_ref, I do not see it should be required to restrict the assignment operator to lvalues.

@mossmaurice Edit: I thought about this. For function_ref there is probably no good use case to assign to rvalues so it is fine to prohibit this explicitly. Same for all other places where I mentioned it.

It should not be done for all classes without considering their purpose, but it is rarely useful I think.

Copy link
Contributor Author

@mossmaurice mossmaurice Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elfenpiff That's due to AUTOSAR rule A12-8-7. Indeed

std::move(a) = b;

is weird and does not make sense. You can find more examples in the AUTOSAR document.

For your 2nd example, I suppose if it is needed in some cases, we should explicitly define foo& operator=(const foo&) && and if desired call operator=() & from there explicitly.

Copy link
Contributor

@MatthiasKillat MatthiasKillat Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both implementations (&& and &) exist and are identical it does not make sense to distinguish. Only if one should not exist (as here) or if they both exist but are different.

It is fine if we only have one like here.


/// @brief Creates a function_ref with a callable whose lifetime has to be longer than function_ref
/// @param[in] callable that is not a function_ref
template <typename CallableType,
typename = std::enable_if_t<!is_function_pointer<CallableType>::value
&& !has_same_decayed_type<CallableType, function_ref>::value
&& is_invocable<CallableType, ArgTypes...>::value>>
function_ref(CallableType&& callable) noexcept;
typename = std::enable_if_t<(!is_function_pointer<CallableType>::value)
&& (!has_same_decayed_type<CallableType, function_ref>::value)
&& (is_invocable<CallableType, ArgTypes...>::value)>>
// AXIVION Next Line AutosarC++19_03-A12.1.4 : Implicit conversion is needed for lambdas
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
function_ref(CallableType&& callable) noexcept; // NOLINT(hicpp-explicit-conversions)

/// @brief Creates a function_ref from a function pointer
/// @param[in] function function reference to function we want to reference
///
/// @note This overload is needed, as the general implementation
/// will not work properly for function pointers.
/// This ctor is not needed anymore once we can use user-defined-deduction guides (C++17)
// Implicit conversion needed for method pointers
// NOLINTNEXTLINE(hicpp-explicit-conversions)
function_ref(ReturnType (&function)(ArgTypes...)) noexcept;

function_ref(function_ref&& rhs) noexcept;

function_ref& operator=(function_ref&& rhs) noexcept;
function_ref& operator=(function_ref&& rhs) & noexcept;
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved

/// @brief Calls the provided callable
/// @param[in] Arguments are forwarded to the underlying function pointer
Expand All @@ -112,9 +114,13 @@ class function_ref<ReturnType(ArgTypes...)>
ReturnType (*m_functionPointer)(void*, ArgTypes...){nullptr};
};

template <class ReturnType, class... ArgTypes>
void swap(function_ref<ReturnType(ArgTypes...)>& lhs, function_ref<ReturnType(ArgTypes...)>& rhs) noexcept;

} // namespace cxx
} // namespace iox

// AXIVION Next Line AutosarC++19_03-M16.0.1 : Include needed to split template declaration and definition
#include "iceoryx_hoofs/internal/cxx/function_ref.inl"

#endif
#endif // IOX_HOOFS_CXX_FUNCTION_REF_HPP
44 changes: 37 additions & 7 deletions iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,34 @@
//
// SPDX-License-Identifier: Apache-2.0

#ifndef IOX_HOOFS_CXX_FUNCTION_REF_INL
#define IOX_HOOFS_CXX_FUNCTION_REF_INL

#include "iceoryx_hoofs/cxx/function_ref.hpp"
#include "iceoryx_hoofs/cxx/requires.hpp"

#include <memory>

namespace iox
{
namespace cxx
{
template <class ReturnType, class... ArgTypes>
template <typename CallableType, typename>
// AXIVION Next Construct AutosarC++19_03-A12.1.2 : Members are initialized in the same manner, NSDMI with nullptr is
// explicit
// AXIVION Next Construct AutosarC++19_03-A8.4.6 : Only ArgTypes needs to be forwarded
inline function_ref<ReturnType(ArgTypes...)>::function_ref(CallableType&& callable) noexcept
// AXIVION Next Construct AutosarC++19_03-A5.2.4, AutosarC++19_03-A5.2.3, CertC++-EXP55 : Type-safety ensured by
// casting back on call
// NOLINTNEXTLINE (cppcoreguidelines-pro-type-reinterpret-cast, cppcoreguidelines-pro-type-const-cast)
: m_pointerToCallable(const_cast<void*>(reinterpret_cast<const void*>(std::addressof(callable))))
// AXIVION Next Line AutosarC++19_03-A15.4.4 : Lambda not 'noexcept' as callable might throw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this argument the call operator must be noexcept.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

15-4-4 states that "A declaration of non-throwing function shall contain noexcept specification.". I have no problem with that. However, any function that might throw cannot have this specificier, which means any user-defined function cannot. Hence I do not see why Axivion should complain unless it is better analyzing whether a function can throw or not than the compiler.

And even modern compilers are in general not able to do this in general, non-trivial cases (like this one). Not sure the rule is useful to enforce with this tool for this reason. If the tool produces too many false positives/negatives it is useless for this rule (while the rule itself makes sense).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elfenpiff I don't get this, the operator()() is already noexcept.

@MatthiasKillat Correct, we need to monitor that.

, m_functionPointer([](void* target, ArgTypes... args) -> ReturnType {
// AXIVION Next Construct AutosarC++19_03-A5.2.4, CertC++-EXP36 : The class design ensures a cast to the actual
// type of target
// AXIVION Next Construct AutosarC++19_03-A5.3.2, AutosarC++19_03-M5.2.8 : Check for 'nullptr' is
// performed on call NOLINTNEXTLINE (cppcoreguidelines-pro-type-reinterpret-cast)
return (*reinterpret_cast<typename std::add_pointer<CallableType>::type>(target))(
std::forward<ArgTypes>(args)...);
})
Expand All @@ -32,16 +51,23 @@ inline function_ref<ReturnType(ArgTypes...)>::function_ref(CallableType&& callab

template <class ReturnType, class... ArgTypes>
inline function_ref<ReturnType(ArgTypes...)>::function_ref(ReturnType (&function)(ArgTypes...)) noexcept
{
// the cast is required to work on POSIX systems
m_pointerToCallable = reinterpret_cast<void*>(function);

// AXIVION Next Construct AutosarC++19_03-A5.2.4, AutosarC++19_03-A5.2.4-M5.2.6 : Type-safety ensured by casting
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
// back function pointer on call
// NOLINTNEXTLINE (cppcoreguidelines-pro-type-reinterpret-cast)
: m_pointerToCallable(reinterpret_cast<void*>(function))
,
// the lambda does not capture and is thus convertible to a function pointer
// (required by the C++ standard)
m_functionPointer = [](void* target, ArgTypes... args) -> ReturnType {
auto f = reinterpret_cast<ReturnType (*)(ArgTypes...)>(target);
m_functionPointer([](void* target, ArgTypes... args) -> ReturnType {
using PointerType = ReturnType (*)(ArgTypes...);
// AXIVION Next Construct AutosarC++19_03-A5.2.4 : The class design ensures a cast to the actual type of target
// NOLINTNEXTLINE (cppcoreguidelines-pro-type-reinterpret-cast)
PointerType f = reinterpret_cast<PointerType>(target);
// AXIVION Next Line AutosarC++19_03-A5.3.2 : Check for 'nullptr' is performed on call
return f(args...);
};
})
{
}

template <class ReturnType, class... ArgTypes>
Expand All @@ -52,7 +78,7 @@ inline function_ref<ReturnType(ArgTypes...)>::function_ref(function_ref&& rhs) n

template <class ReturnType, class... ArgTypes>
inline function_ref<ReturnType(ArgTypes...)>&
function_ref<ReturnType(ArgTypes...)>::operator=(function_ref<ReturnType(ArgTypes...)>&& rhs) noexcept
function_ref<ReturnType(ArgTypes...)>::operator=(function_ref<ReturnType(ArgTypes...)>&& rhs) & noexcept
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
{
if (this != &rhs)
{
Expand All @@ -69,6 +95,7 @@ template <class ReturnType, class... ArgTypes>
inline ReturnType function_ref<ReturnType(ArgTypes...)>::operator()(ArgTypes... args) const noexcept
{
// Expect that a callable was assigned beforehand
// AXIVION Next Line AutosarC++19_03-M5.3.1 : 'nullptr' check shall be performed explicitly
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
cxx::Expects((m_pointerToCallable != nullptr) && "Empty function_ref invoked");
return m_functionPointer(m_pointerToCallable, std::forward<ArgTypes>(args)...);
}
Expand All @@ -81,10 +108,13 @@ inline void function_ref<ReturnType(ArgTypes...)>::swap(function_ref<ReturnType(
}

template <class ReturnType, class... ArgTypes>
// AXIVION Next Line AutosarC++19_03-A2.10.4 : Overload for swap(function_ref, function_ref) as in STL
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
inline void swap(function_ref<ReturnType(ArgTypes...)>& lhs, function_ref<ReturnType(ArgTypes...)>& rhs) noexcept
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
{
lhs.swap(rhs);
}

} // namespace cxx
} // namespace iox

#endif // IOX_HOOFS_CXX_FUNCTION_REF_INL
57 changes: 26 additions & 31 deletions iceoryx_hoofs/test/moduletests/test_cxx_function_ref.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,29 +25,29 @@ namespace
using namespace ::testing;
using namespace iox::cxx;

constexpr int freeFuncTestValue = 42 + 42;
constexpr int functorTestValue = 11;
constexpr int memberFuncTestValue = 4273;
constexpr int sameSignatureIntTestValue = 12345;
constexpr int sameSignatureVoidTestValue = 12346;
constexpr int sameSignatureIntIntTestValue = 12347;
constexpr int FREE_FUNC_TEST_VALUE = 42 + 42;
constexpr int FUNCTOR_TEST_VALUE = 11;
constexpr int MEMBER_FUNC_TEST_VALUE = 4273;
constexpr int SAME_SIGNATURE_INT_TEST_VALUE = 12345;
constexpr int SAME_SIGNATURE_VOID_TEST_VALUE = 12346;
constexpr int SAME_SIGNATURE_INT_INT_TEST_VALUE = 12347;

int freeFunction()
{
return freeFuncTestValue;
return FREE_FUNC_TEST_VALUE;
}

void freeVoidFunction(int& arg)
{
arg = freeFuncTestValue;
arg = FREE_FUNC_TEST_VALUE;
}

class Functor
{
public:
int operator()()
{
return functorTestValue;
return FUNCTOR_TEST_VALUE;
}
};

Expand All @@ -59,14 +59,7 @@ struct ComplexType

bool operator==(const ComplexType& rhs) const
{
if (a == rhs.a && b == rhs.b && c == rhs.c)
{
return true;
}
else
{
return false;
}
return (a == rhs.a && b == rhs.b && c == rhs.c);
}
};

Expand All @@ -77,7 +70,7 @@ ComplexType returnComplexType(ComplexType foo)

int SameSignature(function_ref<int(int)> callback)
{
return callback(sameSignatureIntTestValue);
return callback(SAME_SIGNATURE_INT_TEST_VALUE);
}

int SameSignature(function_ref<int(void)> callback)
Expand All @@ -87,7 +80,7 @@ int SameSignature(function_ref<int(void)> callback)

int SameSignature(function_ref<int(int, int)> callback)
{
return callback(sameSignatureIntIntTestValue, sameSignatureIntIntTestValue);
return callback(SAME_SIGNATURE_INT_INT_TEST_VALUE, SAME_SIGNATURE_INT_INT_TEST_VALUE);
}

class function_refTest : public Test
Expand All @@ -101,9 +94,9 @@ class function_refTest : public Test
{
}

int foobar()
static int foobar()
{
return memberFuncTestValue;
return MEMBER_FUNC_TEST_VALUE;
}

uint8_t m_iter{0};
Expand Down Expand Up @@ -172,6 +165,8 @@ TEST_F(function_refDeathTest, CallMovedFromLeadsToTermination)
auto lambda = []() -> int { return 7654; };
function_ref<int()> sut1{lambda};
function_ref<int()> sut2{std::move(sut1)};
// Use after move is tested here
// NOLINTNEXTLINE (bugprone-use-after-move)
EXPECT_DEATH(sut1(), "Empty function_ref invoked");
}

Expand Down Expand Up @@ -218,7 +213,7 @@ TEST_F(function_refTest, CreateValidWithFreeFunctionResultEqual)
{
::testing::Test::RecordProperty("TEST_ID", "aaf49b6b-054a-4f8f-b176-6d92bb2918da");
function_ref<int()> sut(freeFunction);
EXPECT_THAT(sut(), Eq(freeFuncTestValue));
EXPECT_THAT(sut(), Eq(FREE_FUNC_TEST_VALUE));
}

TEST_F(function_refTest, CreateValidWithComplexTypeResultEqual)
Expand All @@ -234,15 +229,15 @@ TEST_F(function_refTest, CreateValidWithFunctorResultEqual)
::testing::Test::RecordProperty("TEST_ID", "6fd3609b-3254-429a-96ae-20f6dbe99b2a");
Functor foo;
function_ref<int()> sut(foo);
EXPECT_THAT(sut(), Eq(functorTestValue));
EXPECT_THAT(sut(), Eq(FUNCTOR_TEST_VALUE));
}

TEST_F(function_refTest, CreateValidWithStdBindResultEqual)
{
::testing::Test::RecordProperty("TEST_ID", "f5f82896-44db-4d2d-96d0-c1b0fbbe5508");
auto callable = std::bind(&function_refTest::foobar, this);
auto callable = std::bind(&function_refTest::foobar);
function_ref<int()> sut(callable);
EXPECT_THAT(sut(), Eq(memberFuncTestValue));
EXPECT_THAT(sut(), Eq(MEMBER_FUNC_TEST_VALUE));
}

TEST_F(function_refTest, CreateValidWithStdFunctionResultEqual)
Expand All @@ -268,21 +263,21 @@ TEST_F(function_refTest, CallOverloadedFunctionResultsInCallOfInt)
{
::testing::Test::RecordProperty("TEST_ID", "3910ee08-305a-4764-82b3-8b8aa7e7038e");
auto value = SameSignature([](int value) -> int { return value; });
EXPECT_THAT(value, Eq(sameSignatureIntTestValue));
EXPECT_THAT(value, Eq(SAME_SIGNATURE_INT_TEST_VALUE));
}

TEST_F(function_refTest, CallOverloadedFunctionResultsInCallOfVoid)
{
::testing::Test::RecordProperty("TEST_ID", "ca8e8384-0b20-4e4a-b372-698c4e6672b7");
auto value = SameSignature([](void) -> int { return sameSignatureVoidTestValue; });
EXPECT_THAT(value, Eq(sameSignatureVoidTestValue));
auto value = SameSignature([](void) -> int { return SAME_SIGNATURE_VOID_TEST_VALUE; });
EXPECT_THAT(value, Eq(SAME_SIGNATURE_VOID_TEST_VALUE));
}

TEST_F(function_refTest, CallOverloadedFunctionResultsInCallOfIntInt)
{
::testing::Test::RecordProperty("TEST_ID", "b37158b6-8100-4f80-bd62-d2957a7d9c46");
auto value = SameSignature([](int value1, int value2 IOX_MAYBE_UNUSED) -> int { return value1; });
EXPECT_THAT(value, Eq(sameSignatureIntIntTestValue));
EXPECT_THAT(value, Eq(SAME_SIGNATURE_INT_INT_TEST_VALUE));
}

TEST_F(function_refTest, CreationWithFunctionPointerWorks)
Expand All @@ -292,7 +287,7 @@ TEST_F(function_refTest, CreationWithFunctionPointerWorks)
function_ref<int(void)> sut(fp);

auto result = sut();
EXPECT_EQ(result, freeFuncTestValue);
EXPECT_EQ(result, FREE_FUNC_TEST_VALUE);
}

TEST_F(function_refTest, CreationWithFunctionPointerWithRefArgWorks)
Expand All @@ -303,7 +298,7 @@ TEST_F(function_refTest, CreationWithFunctionPointerWithRefArgWorks)

int arg{0};
sut(arg);
EXPECT_EQ(arg, freeFuncTestValue);
EXPECT_EQ(arg, FREE_FUNC_TEST_VALUE);
}

TEST_F(function_refTest, CreationWithFunctionPointerWithComplexTypeArgWorks)
Expand Down