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.

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 #86 introduce cxx function ref #124

Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
46c591c
iox-#86 Add first version of cxx::function_ref
mossmaurice Apr 20, 2020
eb0521d
iox-#86 Remove commented-out code and fix typos
mossmaurice Apr 20, 2020
7aa40ae
iox-#86 Add failure handling when calling an empty function_ref
mossmaurice Apr 20, 2020
299ba47
iox-#86 Properly initialise the pointers in function_ref
mossmaurice Apr 20, 2020
daf6539
iox-#86 Add function_ref::swap() and its test case
mossmaurice Apr 20, 2020
f7eda93
iox-#86 Add functor and free function test cases to function_ref
mossmaurice Apr 20, 2020
5eedc2c
iox-#86 Allow copy assignment, add its respective test case and add m…
mossmaurice Apr 20, 2020
f7b836f
iox-#86 Add review comment by Mathias Kraus
mossmaurice May 6, 2020
23781bb
iox-#86 Refactor std::enable_if
mossmaurice May 7, 2020
3144f74
iox-#86 Fix type trait and remove todo's
mossmaurice May 20, 2020
ec10108
iox-#86 Add two more test cases for function_ref
mossmaurice May 26, 2020
7d213f5
iox-#86 Add move c'tor to cxx::function_ref
mossmaurice Jun 2, 2020
fd244d6
iox-#86 Add ComplexType test case for function_ref
mossmaurice Jun 2, 2020
280ef2b
iox-#86 Add test case for move c'tor and move assign for function_ref
mossmaurice Jun 2, 2020
7cf3b64
iox-#86 Remove obsolete todo
mossmaurice Jun 2, 2020
f908a39
iox-#86 Replace pragma once with include guard and add explicit overl…
mossmaurice Jun 3, 2020
265a4c1
iox-#86 Fix typo
mossmaurice Jun 3, 2020
411e4a9
iox-#86 Revert move assignment overloads
mossmaurice Jun 3, 2020
3ce323b
iox-#86 Move implementation to function_ref.inl
mossmaurice Jun 4, 2020
4cdde2f
iox-#86 Address review findings
mossmaurice Jun 4, 2020
357fa48
iox-#86 Address review findings in function_ref unit test
mossmaurice Jun 5, 2020
e76d580
iox-#86 Add missing newline
mossmaurice Jun 8, 2020
1a5d405
iox-#86 Address doxygen review findings
mossmaurice Jun 9, 2020
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
155 changes: 155 additions & 0 deletions iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
// Copyright (c) 2020 by Robert Bosch GmbH. All rights reserved.
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add @param[in], @return in doxygen comments and remove unnecessary comments

//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef IOX_UTILS_CXX_FUNCTION_REF_HPP
#define IOX_UTILS_CXX_FUNCTION_REF_HPP

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

namespace iox
{
namespace cxx
{
/// @brief cxx::function_ref is a non-owning reference to a callable.
///
/// It has these features:
/// * No heap usage
/// * No exceptions
/// * Stateful lambda support
/// * C++11/14 support
/// @code
/// // Usage as function parameter
/// void fuu(cxx::function_ref<void()> callback)
/// {
/// callback();
/// }
/// // Call the lambda
/// fuu([]{ doSomething(); });
///
/// // Usage with l-values
/// // Pitfall: Ensure that lifetime of callable suits the point in time of calling callback()
Copy link
Contributor

Choose a reason for hiding this comment

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

This pitfall has to be stated in the doxygen docu in the method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// auto callable = [&]{ doSomething(); };
/// cxx::function_ref<void()> callback(callable);
/// // Call the callback
/// callback();
/// @endcode

template <typename SignatureType>
class function_ref;

template <class ReturnType, class... ArgTypes>
class function_ref<ReturnType(ArgTypes...)>
{
using SignatureType = ReturnType(ArgTypes...);
using SelfType = function_ref<SignatureType>;
template <typename T>
using EnableIfNotFunctionRef = typename std::enable_if<!std::is_same<std::decay<T>, function_ref>::value>::type;

public:
/// @brief Creates an empty function_ref
Copy link
Contributor

Choose a reason for hiding this comment

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

You should maybe state the default constructor creates a function_ref in an invalid state and operator bool() returns false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

function_ref() noexcept
Copy link
Member

Choose a reason for hiding this comment

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

not really a big fan of this. optional<function_ref> would be better in my point of view, but it seems the decision was to do it this way

on the other hand, you would result in the same, invalid object after a move operation

mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
: m_target(nullptr)
, m_functionPointer(nullptr)
{
}

/// @brief Creates an empty function_ref
function_ref(nullptr_t) noexcept
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
: function_ref()
{
}

/// @brief D'tor
Copy link
Contributor

Choose a reason for hiding this comment

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

Please write a meaningful comment or remove it. That this is the default destructor is obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Touché

~function_ref() noexcept = default;

/// @brief Copy c'tor
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do not need to document compiler generated code. This is the default copy constructor which is already clearly declared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your Karma point +1 ;)

function_ref(const function_ref&) noexcept = default;

/// @brief Copy assignment operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove unnecessary comment

function_ref& operator=(const function_ref&) noexcept = default;

/// @brief Create a function_ref
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
template <typename CallableType, typename = EnableIfNotFunctionRef<SelfType>>
function_ref(CallableType&& callable) noexcept
: m_target(reinterpret_cast<void*>(std::addressof(callable)))
, m_functionPointer([](void* target, ArgTypes... args) -> ReturnType {
return (*reinterpret_cast<typename std::add_pointer<CallableType>::type>(target))(
std::forward<ArgTypes>(args)...);
})
{
}

/// @brief Moves a function_ref
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove unnecessary comment!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

function_ref(function_ref&& rhs) noexcept
{
*this = std::move(rhs);
}

/// @brief Move assignment operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove unnecessary comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

function_ref& operator=(function_ref&& rhs) noexcept
{
if (this != &rhs)
{
m_target = rhs.m_target;
m_functionPointer = rhs.m_functionPointer;
// Make sure no UB can happen by marking the lvalue as invalid
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
rhs.m_target = nullptr;
rhs.m_functionPointer = nullptr;
}
return *this;
};

/// @brief Calls the provided callable
ReturnType operator()(ArgTypes... args) const noexcept
{
if (!m_target)
{
// Callable was called without user having assigned one beforehand
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
std::terminate();
}
return m_functionPointer(m_target, std::forward<ArgTypes>(args)...);
}

/// @brief Checks whether a valid target is contained
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for completeness, I would add @return true if valid target is contained, otherwise false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

explicit operator bool() const noexcept
{
return m_target != nullptr;
}

void swap(function_ref& rhs) noexcept
{
std::swap(m_target, rhs.m_target);
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
std::swap(m_functionPointer, rhs.m_functionPointer);
}

private:
/// @brief Raw pointer of the callable
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not document private variables. Otherwise we are ending up with comments which have nothing todo with the actual code. And when this is the raw pointer of the callable maybe we could rename it into: m_ptrToCallable so that no more comment is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Valid point, renamed it.

void* m_target{nullptr};

/// @brief Function pointer to the callable
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, remove the comment since the name is clearly understandable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

ReturnType (*m_functionPointer)(void*, ArgTypes...){nullptr};
};

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

} // namespace cxx
} // namespace iox

#endif
228 changes: 228 additions & 0 deletions iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
// Copyright (c) 2020 by Robert Bosch GmbH. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "iceoryx_utils/cxx/function_ref.hpp"
#include "test.hpp"

using namespace ::testing;
using namespace iox::cxx;

int freeFunction()
{
return 42 + 42;
}

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

struct ComplexType
{
char a;
int b;
float c;

bool operator==(const ComplexType& rhs) const
{
if (a == rhs.a && b == rhs.b && c == rhs.c)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not write simply:

return ( a == rhs.a && b == rhs.b && c == rhs.c);

This would save you 7 lines and lines these days are very expensive ;)

Copy link
Member

Choose a reason for hiding this comment

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

that's boring ... on the other side, Walter E. Brown knows much more interesting ways to return true ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is test code I don't care about LoC, but prefer to make it more readable :-) For everyone interested in the brilliant talk, here's the link: https://www.youtube.com/watch?v=OAmWHmwlMwI

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree. How does it make your code more readable when you just add more lines?
The first line ( a == rhs.a && b == rhs.b && c == rhs.c) have to be understood either way and with your approach you have 6 more lines to understood

{
return true;
}
else
{
return false;
}
}
};

ComplexType returnComplexType(ComplexType foo)
{
return foo;
}

class function_refTest : public Test
{
public:
void SetUp() override
{
}

void TearDown() override
{
}

int foobar()
{
return 4273;
}

uint8_t m_iter{0};
};

using function_refDeathTest = function_refTest;

TEST_F(function_refTest, CreateEmpty)
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
{
function_ref<void()> sut;
EXPECT_FALSE(sut);
}

TEST_F(function_refTest, CreateWithNullptr)
{
function_ref<void()> sut(nullptr);
EXPECT_FALSE(sut);
}

TEST_F(function_refDeathTest, CreateEmptyLeadsToTermination)
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
{
function_ref<void()> sut;
EXPECT_DEATH(sut(), ".*");
}

TEST_F(function_refTest, CreateEmptyAndAssign)
{
auto lambda = [] {};
function_ref<void()> sut;
sut = lambda;
EXPECT_TRUE(sut);
}

TEST_F(function_refTest, CreateAndCopy)
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
{
auto lambda = []() -> int { return 42; };
function_ref<int()> sut1(lambda);
function_ref<int()> sut2{sut1};
EXPECT_TRUE(sut2);
Copy link
Member

Choose a reason for hiding this comment

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

please make this an ASSERT_TRUE, else the whole test will terminate in the next call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be done in follow-up #131

EXPECT_THAT(sut2(), Eq(42));
Copy link
Member

Choose a reason for hiding this comment

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

please use a constexpr instead of magic numbers

Copy link
Contributor Author

@mossmaurice mossmaurice Jun 8, 2020

Choose a reason for hiding this comment

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

Magic numbers are magic 🦄 For small tests I prefer to omit the extra variable.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree, but not enough to prevent this merge

it should become muscle memory and that only happens if you do it all the time ;)

}

TEST_F(function_refTest, CreateAndCopyAssign)
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
{
auto lambda = []() -> int { return 42; };
function_ref<int()> sut2;
{
function_ref<int()> sut1(lambda);
EXPECT_THAT(sut1(), Eq(42));
EXPECT_FALSE(sut2);
sut2 = sut1;
}
EXPECT_TRUE(sut2);
EXPECT_THAT(sut2(), Eq(42));
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
}

TEST_F(function_refTest, CreateAndMove)
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
{
auto lambda = []() -> int { return 42; };
function_ref<int()> sut1{lambda};
function_ref<int()> sut2(std::move(sut1));
EXPECT_TRUE(sut2);
Copy link
Member

Choose a reason for hiding this comment

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

please make an ASSERT_THAT, else the access to sut2 further down might terminate the whole test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will be done in #131

EXPECT_THAT(sut2(), Eq(42));
EXPECT_FALSE(sut1);
}

TEST_F(function_refTest, CreateAndMoveAssign)
{
auto lambda1 = []() -> int { return 42; };
auto lambda2 = []() -> int { return 73; };
function_ref<int()> sut1{lambda1};
{
function_ref<int()> sut2{lambda2};
sut1 = std::move(sut2);
}
EXPECT_TRUE(sut1);
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_THAT(sut1(), Eq(73));
}

TEST_F(function_refTest, CreateAndSwap)
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
{
auto lambda1 = []() -> int { return 42; };
auto lambda2 = []() -> int { return 73; };
function_ref<int()> sut1(lambda1);
function_ref<int()> sut2(lambda2);
EXPECT_THAT(sut1(), Eq(42));
EXPECT_THAT(sut2(), Eq(73));
sut1.swap(sut2);
EXPECT_THAT(sut1(), Eq(73));
EXPECT_THAT(sut2(), Eq(42));
}

TEST_F(function_refTest, CreateWithCapturingLambdaVoidVoid)
{
auto lambda = [&] { m_iter++; };
function_ref<void(void)> sut(lambda);
sut();
EXPECT_THAT(m_iter, Eq(1));
}

TEST_F(function_refTest, CreateWithLambdaIntVoid)
{
auto lambda = [](void) -> int { return 42; };
function_ref<int(void)> sut(lambda);
EXPECT_THAT(sut(), Eq(42));
}

TEST_F(function_refTest, CreateWithLambdaIntInt)
{
auto lambda = [](int var) -> int { return ++var; };
function_ref<int(int)> sut(lambda);
EXPECT_THAT(sut(m_iter), Eq(1));
}

TEST_F(function_refTest, CreateWithFreeFunction)
{
function_ref<int()> sut(freeFunction);
EXPECT_THAT(sut(), Eq(84));
}

TEST_F(function_refTest, CreateWithComplexType)
{
ComplexType fuubar{1, 2, 1.3f};
function_ref<ComplexType(ComplexType)> sut(returnComplexType);
EXPECT_THAT(sut(fuubar), Eq(fuubar));
}

TEST_F(function_refTest, CreateWithFunctor)
{
Functor foo;
function_ref<int()> sut(foo);
EXPECT_THAT(sut(), Eq(73));
}

TEST_F(function_refTest, CreateWithStdBind)
{
function_ref<int()> sut(std::bind(&function_refTest::foobar, this));
EXPECT_THAT(sut(), Eq(4273));
Copy link
Member

Choose a reason for hiding this comment

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

no magic numbers please

}

TEST_F(function_refTest, CreateWithStdFunction)
{
std::function<int()> baz;
baz = []() -> int { return 24; };
function_ref<int()> sut(baz);
EXPECT_THAT(sut(), Eq(24));
}

TEST_F(function_refTest, StoreInStdFunction)
{
auto lambda = []() -> int { return 37; };
function_ref<int()> moep(lambda);
// Moves cxx::function_ref into std::function, needs copy c'tor
std::function<int()> sut(moep);
EXPECT_THAT(sut(), Eq(37));
}