Iox #86 introduce cxx function ref #124

mossmaurice · 2020-06-02T14:46:06Z

Resolves #86

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…ction_ref Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…nction_ref Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

… case and add more test todo's Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

…or function_ref Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

… explicit overloads for move assignment operator Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

elBoberido · 2020-06-03T15:13:13Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

+
+  public:
+    /// @brief Creates an empty function_ref
+    function_ref() noexcept


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

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

elBoberido · 2020-06-03T15:34:00Z

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

+    function_ref<int()> sut1(lambda);
+    function_ref<int()> sut2{sut1};
+    EXPECT_TRUE(sut2);
+    EXPECT_THAT(sut2(), Eq(42));


please use a constexpr instead of magic numbers

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

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 ;)

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

elBoberido · 2020-06-03T15:46:37Z

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

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


no magic numbers please

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

elfenpiff · 2020-06-03T15:36:20Z

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

+
+    bool operator==(const ComplexType& rhs) const
+    {
+        if (a == rhs.a && b == rhs.b && c == rhs.c)


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 ;)

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

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

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

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

budrus · 2020-06-08T15:15:30Z

iceoryx_utils/include/iceoryx_utils/internal/cxx/function_ref.inl

+    if (!m_target)
+    {
+        // Callable was called without user having assigned one beforehand
+        std::cerr << "Empty function_ref invoked" << std::endl;


I think we have to come up with a strategy whether is is cool to directly use std::cerr or if we should use our logger. As this is not the first affected module, this PR is fine so far but I created a new issue to address this question #130

the idea was to use cout, clog and cerr in the utils and then let the logger capture that and map it to a LogInfo, LogWarn and LogError ... unfortunatelly there was not yet time to do this

elBoberido · 2020-06-09T10:20:09Z

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

+    EXPECT_TRUE(sut);
+}
+
+TEST_F(function_refTest, CreateValidByAssignResultEqual)


shouldn't this also be CallValid...

elBoberido · 2020-06-09T10:20:32Z

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

+    EXPECT_THAT(sut(), Eq(7253));
+}
+
+TEST_F(function_refTest, CreateValidByCopyConstructResultEqual)


again, CallValid...

I'll rename it just for you <3

elBoberido · 2020-06-09T10:23:24Z

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

+    auto lambda = []() -> int { return 3527; };
+    function_ref<int()> sut1{lambda};
+    function_ref<int()> sut2(sut1);
+    EXPECT_TRUE(sut2);


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

Will be done in follow-up #131

elBoberido · 2020-06-09T10:31:25Z

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

+    auto lambda = []() -> int { return 123; };
+    function_ref<int()> sut1{lambda};
+    function_ref<int()> sut2{std::move(sut1)};
+    EXPECT_TRUE(sut2);


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

Good point, will be done in #131

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

elfenpiff

Remove unnecessary comments.
Add some more tags to your comments.

elfenpiff · 2020-06-09T11:43:10Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

+    /// @brief Creates an empty function_ref
+    function_ref() noexcept;
+
+    /// @brief D'tor


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

elfenpiff · 2020-06-09T11:44:08Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

+    /// @brief D'tor
+    ~function_ref() noexcept = default;
+
+    /// @brief Copy c'tor


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

Your Karma point +1 ;)

elfenpiff · 2020-06-09T11:44:23Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

+    /// @brief Copy c'tor
+    function_ref(const function_ref&) noexcept = default;
+
+    /// @brief Copy assignment operator


Please remove unnecessary comment

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

elfenpiff · 2020-06-09T11:46:14Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

+    template <typename CallableType, typename = EnableIfNotFunctionRef<CallableType>>
+    function_ref(CallableType&& callable) noexcept;
+
+    /// @brief Moves a function_ref


Please remove unnecessary comment!

elfenpiff · 2020-06-09T11:54:42Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

+    /// @brief Raw pointer of the callable
+    void* m_target{nullptr};
+
+    /// @brief Function pointer to the callable


Again, remove the comment since the name is clearly understandable

elfenpiff · 2020-06-09T11:55:16Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

+    /// @brief Checks whether a valid target is contained
+    explicit operator bool() const noexcept;
+
+    void swap(function_ref& rhs) noexcept;


This function is not documented...

Added documentation

elfenpiff · 2020-06-09T11:55:37Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

@@ -0,0 +1,105 @@
+// Copyright (c) 2020 by Robert Bosch GmbH. All rights reserved.


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

elfenpiff · 2020-06-09T12:02:42Z

iceoryx_utils/test/moduletests/test_cxx_function_ref.cpp

+
+    bool operator==(const ComplexType& rhs) const
+    {
+        if (a == rhs.a && b == rhs.b && c == rhs.c)


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

elfenpiff · 2020-06-09T12:12:23Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

+///         fuu([]{ doSomething(); });
+///
+///         // Usage with l-values
+///         // Pitfall: Ensure that lifetime of callable suits the point in time of calling callback()


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

elfenpiff · 2020-06-09T12:16:23Z

iceoryx_utils/include/iceoryx_utils/cxx/function_ref.hpp

+    using EnableIfNotFunctionRef = typename std::enable_if<!std::is_same<std::decay<T>, function_ref>::value>::type;
+
+  public:
+    /// @brief Creates an empty function_ref


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

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

mossmaurice added 14 commits April 20, 2020 12:10

iox-eclipse-iceoryx#86 Add first version of cxx::function_ref

46c591c

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Remove commented-out code and fix typos

eb0521d

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Add failure handling when calling an empty fun…

7aa40ae

…ction_ref Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Properly initialise the pointers in function_ref

299ba47

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Add function_ref::swap() and its test case

daf6539

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Add functor and free function test cases to fu…

f7eda93

…nction_ref Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Allow copy assignment, add its respective test…

5eedc2c

… case and add more test todo's Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Add review comment by Mathias Kraus

f7b836f

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Refactor std::enable_if

23781bb

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Fix type trait and remove todo's

3144f74

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Add two more test cases for function_ref

ec10108

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Add move c'tor to cxx::function_ref

7d213f5

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Add ComplexType test case for function_ref

fd244d6

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Add test case for move c'tor and move assign f…

280ef2b

…or function_ref Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

mossmaurice added the enhancement New feature label Jun 2, 2020

mossmaurice requested review from elBoberido, elfenpiff and budrus June 2, 2020 14:46

mossmaurice added 4 commits June 2, 2020 16:57

iox-eclipse-iceoryx#86 Remove obsolete todo

7cf3b64

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Replace pragma once with include guard and add…

f908a39

… explicit overloads for move assignment operator Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Fix typo

265a4c1

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Revert move assignment overloads

411e4a9

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

elBoberido requested changes Jun 3, 2020

View reviewed changes

elfenpiff requested changes Jun 3, 2020

View reviewed changes

mossmaurice added 4 commits June 4, 2020 17:43

iox-eclipse-iceoryx#86 Move implementation to function_ref.inl

3ce323b

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Address review findings

4cdde2f

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Address review findings in function_ref unit test

357fa48

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

iox-eclipse-iceoryx#86 Add missing newline

e76d580

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

budrus previously approved these changes Jun 8, 2020

View reviewed changes

elBoberido requested changes Jun 9, 2020

View reviewed changes

elfenpiff requested changes Jun 9, 2020

View reviewed changes

elfenpiff reviewed Jun 9, 2020

View reviewed changes

iox-eclipse-iceoryx#86 Address doxygen review findings

1a5d405

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

mossmaurice dismissed budrus’s stale review via 1a5d405 June 9, 2020 13:12

elfenpiff approved these changes Jun 9, 2020

View reviewed changes

elBoberido approved these changes Jun 9, 2020

View reviewed changes

mossmaurice merged commit ca6a790 into eclipse-iceoryx:master Jun 9, 2020

mossmaurice added a commit to mossmaurice/iceoryx that referenced this pull request Jun 10, 2020

iox-eclipse-iceoryx#131 Address leftover findings of eclipse-iceoryx#124

e7fbc3e

Signed-off-by: Hoinkis Simon (CC-AD/ESW1) <simon.hoinkis2@de.bosch.com>

		@@ -0,0 +1,105 @@
		// Copyright (c) 2020 by Robert Bosch GmbH. All rights reserved.

Iox #86 introduce cxx function ref #124

Iox #86 introduce cxx function ref #124

Conversation

mossmaurice commented Jun 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmaurice Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elfenpiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmaurice Jun 8, 2020 •

edited

Loading