iox-#186 Add first cxx::forward_list implementation to utils #189

mossmaurice · 2020-07-13T11:18:25Z

Code contributed by @bishibashiB

… utils Signed-off-by: Bernhard Eickhoff (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

iceoryx_utils/test/moduletests/test_cxx_forward_list.cpp

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

elfenpiff

Remove the two distinct unrelated iterator classes and just write one iterator class with const overloaded methods.
They can be aliased like

using iterator = MyIteratorClass;
using const_iterator = const MyIteratorClass;

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

elfenpiff · 2020-07-14T09:05:14Z

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

+    };
+
+    void init() noexcept;
+    ListNodeType* getNodePtrFromIdx(sizeType) const noexcept;


Please return const ListNodeType * since this method is const and we do not want to change the internal state.

~~I guess all three internal functions (below) cast away constness from a (potential) const list to non-const node member pointer. Themself they do not change anything on the list.~~

ListNodeType* getNodePtrFromIdx(SizeType) const noexcept; ListNodeType* getHeadFreeEl() const noexcept; ListNodeType* getPointerToListNode() const noexcept;

For a better internal handling of constness I wonder if it would be cleaner to replace all three with a const & non-const tuple, like so

ListNodeType* getNodePtrFromIdx(SizeType) noexcept; const ListNodeType* getNodePtrFromIdx(SizeType) const noexcept; ListNodeType* getHeadFreeEl() noexcept; const ListNodeType* getHeadFreeEl() const noexcept; ListNodeType* getPointerToListNode() noexcept; const ListNodeType* getPointerToListNode() const noexcept;

~~Upper (current) implementation is slimmer. Would the later anyway be appreciated? Am I missing other alternatives?~~
Updated to follow Effective C++ Item3.

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

iceoryx_utils/test/moduletests/test_cxx_forward_list.cpp

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

…lCase, prefix and namings/comments (review findings) Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

…PECT_DEATH test on mac Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

MatthiasKillat · 2020-07-15T15:27:23Z

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

+    if (!isValidElementIndex(iter.m_iterListNodeIdx))
+    {
+        errorMessage(__PRETTY_FUNCTION__, " tryed accessing not existing element");
+        std::terminate();


undefined behaviour in C++ so terminate might be justified (but not necessary since we already checked for it, crash/terminate in undefined behaviour often happens since the check is skipped for efficiency reasons...). This is again the question of how to deal with certain kinds of usage-errors.

see above, directive for aligned error handling

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

…out-of-class operator==, type can format review findings Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

…d_cxx_forward_list Hopefully fixing the windows build

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

mossmaurice · 2020-07-20T14:17:04Z

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

+// template <typename IterType, typename IterTypeOther>
+// bool operator==<typename std::enable_if<is_forward_list_iterator<IterType>::value>::type,
+//                 typename std::enable_if<is_forward_list_iterator<IterTypeOther>::value>::type>(
+//     const IterType& lhs_iter, const IterTypeOther& rhs_iter) noexcept;


uups, sure! removed in 6bbaaffef5d63b6e7ff0785d850bb02c93ba638b

marthtz · 2020-07-23T08:13:10Z

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

+// limitations under the License.
+
+#ifndef CXX_FORWARD_LIST_HPP_INCLUDED
+#define CXX_FORWARD_LIST_HPP_INCLUDED


Include guard should look like this "IOX_UTILS_CXX_FORWARD_LIST_HPP"

done in 80f54bf97462be9eb59d71c1dfabdc2beab54efd

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

bishibashiB · 2020-08-03T18:31:11Z

iceoryx_utils/readme.md shall provide a hint regarding the forward_list implementation.

…fix comments Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

…rite), add remove(_if) member_function, correct include guards Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

…ntation Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

…ions Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

dkroenke · 2020-08-14T10:01:25Z

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

+}
+
+// iterator
+/*************************/


It's just nitpicking that these separator lines are somehow weird. But i know that's for better readibility. Maybe just add some spacing and then just a comment like you did without these separator lines is sufficient here. But as @elBoberido would say: I have no strong feelings about this. ;-)

So you are from the neutral planet https://www.youtube.com/watch?v=CxK_nA2iVXw. And so am I in this regard. :-)

To respect your non-feelings here: removed

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

bishibashiB · 2020-08-14T13:48:23Z

Remove the two distinct unrelated iterator classes and just write one iterator class with const overloaded methods.
They can be aliased like
using iterator = MyIteratorClass;
using const_iterator = const MyIteratorClass;

see latest implementation using a iterator_base class.

MatthiasKillat · 2020-08-31T09:44:57Z

iceoryx_utils/README.md

@@ -41,11 +41,12 @@ class should be used.
 |`algorithm`          |   |   | Implements `min` and `max` for an arbitrary number of values of the same type. For instance `min(1,2,3,4,5);` |
 |`convert`            |   |   | Converting a number into a string is easy, converting it back can be hard. You can use functions like `strtoll` but you still have to handle errors like under- and overflow, or converting invalid strings into number. Here we abstract all the error handling so that you can convert strings into numbers safely. |
 |`expected`           |   |   | Our base class used in error handling. Every function which can fail should return an expected. With this the user knows that this function can fail and that they have to do some kind of error handling. We got inspired by the [C++ expected proposal]( http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0323r7.html) and by the [rust error handling concept](https://doc.rust-lang.org/std/result/enum.Result.html). |
+|`forward_list`       |   |   | Heap and exception free implementation of `std::forward_list` |


which can be stored in shared memory or is relocatable (could be mentioned optionally).

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

iceoryx_utils/test/moduletests/test_cxx_forward_list.cpp

MatthiasKillat · 2020-09-01T07:42:25Z

iceoryx_utils/test/moduletests/test_cxx_forward_list.cpp

+{
+    for (uint64_t i = 0; i < TESTLISTCAPACITY; ++i)
+    {
+        sut.emplace_front((const uint64_t)i);


remove C-style cast

iceoryx_utils/test/moduletests/test_cxx_forward_list.cpp

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

bishibashiB · 2020-09-01T09:01:59Z

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

+    struct NodeLink
+    {
+        size_type nextIdx;
+        bool freedElement;


bishibashiB · 2020-09-01T09:03:11Z

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

+    static constexpr size_type END_INDEX{size_type(Capacity) + 1U};
+    static constexpr size_type NODE_LINK_COUNT{size_type(Capacity) + 2U};
+
+    // two member variables point to head of freeList and usedList


iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

bishibashiB · 2020-09-01T09:21:43Z

iceoryx_utils/README.md

@@ -41,11 +41,12 @@ class should be used.
 |`algorithm`          |   |   | Implements `min` and `max` for an arbitrary number of values of the same type. For instance `min(1,2,3,4,5);` |
 |`convert`            |   |   | Converting a number into a string is easy, converting it back can be hard. You can use functions like `strtoll` but you still have to handle errors like under- and overflow, or converting invalid strings into number. Here we abstract all the error handling so that you can convert strings into numbers safely. |
 |`expected`           |   |   | Our base class used in error handling. Every function which can fail should return an expected. With this the user knows that this function can fail and that they have to do some kind of error handling. We got inspired by the [C++ expected proposal]( http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0323r7.html) and by the [rust error handling concept](https://doc.rust-lang.org/std/result/enum.Result.html). |
+|`forward_list`       |   |   | Heap and exception free implementation of `std::forward_list` |


iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

iceoryx_utils/test/moduletests/test_cxx_forward_list.cpp

bishibashiB · 2020-09-01T19:47:57Z

iceoryx_utils/test/moduletests/test_cxx_forward_list.cpp

+{
+    for (uint64_t i = 0; i < TESTLISTCAPACITY; ++i)
+    {
+        sut.emplace_front((const uint64_t)i);


iceoryx_utils/test/moduletests/test_cxx_forward_list.cpp

…emcpy Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

iceoryx_utils/include/iceoryx_utils/internal/cxx/forward_list.inl

iceoryx_utils/test/moduletests/test_cxx_forward_list.cpp

dkroenke · 2020-09-03T08:15:36Z

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp

+
+#include "iceoryx_utils/internal/cxx/forward_list.inl"
+
+#endif // IOX_UTILS_CXX_FORWARD_LIST_HPP


Add empty line please

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

Review points are adressed

iox-eclipse-iceoryx#186 add first cxx::forward_list implementation to…

542e566

… utils Signed-off-by: Bernhard Eickhoff (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

mossmaurice requested review from budrus, elfenpiff and elBoberido July 13, 2020 11:18

mossmaurice linked an issue Jul 13, 2020 that may be closed by this pull request

Add cxx::list to utils #186

Closed

mossmaurice added the enhancement New feature label Jul 13, 2020

mossmaurice commented Jul 13, 2020

View reviewed changes

orecham suggested changes Jul 13, 2020

View reviewed changes

elfenpiff previously requested changes Jul 14, 2020

View reviewed changes

iox-eclipse-iceoryx#186 add missing return to insert_after, UpperCame…

e9bdd58

…lCase, prefix and namings/comments (review findings) Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

dkroenke reviewed Jul 14, 2020

View reviewed changes

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp Outdated Show resolved Hide resolved

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp Outdated Show resolved Hide resolved

iox-eclipse-iceoryx#186 review findings, .front() member function, EX…

473f4f8

…PECT_DEATH test on mac Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

orecham reviewed Jul 15, 2020

View reviewed changes

iceoryx_utils/include/iceoryx_utils/cxx/forward_list.hpp Outdated Show resolved Hide resolved

iox-eclipse-iceoryx#186 remove magic number in init(), fix windows build

096b981

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

MatthiasKillat reviewed Jul 15, 2020

View reviewed changes

bishibashiB added 2 commits July 16, 2020 06:53

iox-eclipse-iceoryx#186 improve list and const_iterator constructor, …

2984883

…out-of-class operator==, type can format review findings Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

Merge remote-tracking branch 'origin/master' into feature/iox-#186_ad…

ca9f775

…d_cxx_forward_list Hopefully fixing the windows build

mossmaurice commented Jul 20, 2020

View reviewed changes

mossmaurice requested review from dkroenke and elfenpiff July 20, 2020 14:24

orecham previously approved these changes Jul 21, 2020

View reviewed changes

bishibashiB dismissed orecham’s stale review via 6bbaaff July 22, 2020 08:13

marthtz requested changes Jul 23, 2020

View reviewed changes

bishibashiB mentioned this pull request Jul 30, 2020

Iox #186 Add cxx::list implementation to utils #215

Merged

bishibashiB added 2 commits August 14, 2020 08:32

iox-eclipse-iceoryx#186 inline file include in paranthesis, remove / …

375568c

…fix comments Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

iox-eclipse-iceoryx#186 rework comparison operator (no cxx-wide overw…

b070d0a

…rite), add remove(_if) member_function, correct include guards Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

bishibashiB force-pushed the feature/iox-#186_add_cxx_forward_list branch from 80f54bf to b070d0a Compare August 14, 2020 06:35

bishibashiB added 2 commits August 14, 2020 00:30

iox-eclipse-iceoryx#186 rework to align design with cxx::list impleme…

259e70a

…ntation Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

iox-eclipse-iceoryx#186 merge iterator and const_iterator implementat…

88027ba

…ions Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

dkroenke reviewed Aug 14, 2020

View reviewed changes

Merge branch 'master' into feature/iox-#186_add_cxx_forward_list

bd0883b

MatthiasKillat requested changes Sep 1, 2020

View reviewed changes

iox-eclipse-iceoryx#186 review findings

fc28301

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

bishibashiB reviewed Sep 2, 2020

View reviewed changes

bishibashiB added 3 commits September 2, 2020 06:11

iox-eclipse-iceoryx#186 add forward_list test case ListIsCopyableViaM…

43fb2f0

…emcpy Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

iox-eclipse-iceoryx#186 fix test case ListIsCopyableViaMemcpy

a3a7fec

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

iox-eclipse-iceoryx#186: backport review findings from cxx::list

259ada1

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

MatthiasKillat previously approved these changes Sep 3, 2020

View reviewed changes

dkroenke requested changes Sep 3, 2020

View reviewed changes

iox-eclipse-iceoryx#186: add new line at the end of forward_list.hpp

32ac82d

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

bishibashiB dismissed MatthiasKillat’s stale review via 32ac82d September 3, 2020 08:33

dkroenke self-requested a review September 3, 2020 08:34

dkroenke approved these changes Sep 3, 2020

View reviewed changes

MatthiasKillat approved these changes Sep 3, 2020

View reviewed changes

MatthiasKillat merged commit 9d70ac9 into eclipse-iceoryx:master Sep 3, 2020

bishibashiB deleted the feature/iox-#186_add_cxx_forward_list branch September 3, 2020 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#186 Add first cxx::forward_list implementation to utils #189

iox-#186 Add first cxx::forward_list implementation to utils #189

mossmaurice commented Jul 13, 2020

elfenpiff left a comment

elfenpiff Jul 14, 2020

bishibashiB Jul 15, 2020 •

edited

MatthiasKillat Jul 15, 2020

bishibashiB Jul 16, 2020

mossmaurice Jul 20, 2020

bishibashiB Jul 22, 2020

marthtz Jul 23, 2020

bishibashiB Jul 27, 2020

bishibashiB commented Aug 3, 2020

dkroenke Aug 14, 2020

MatthiasKillat Aug 31, 2020

bishibashiB Sep 1, 2020

bishibashiB commented Aug 14, 2020

MatthiasKillat Aug 31, 2020

bishibashiB Sep 1, 2020

MatthiasKillat Sep 1, 2020

bishibashiB Sep 1, 2020

bishibashiB Sep 1, 2020

bishibashiB Sep 1, 2020

bishibashiB Sep 1, 2020

bishibashiB Sep 1, 2020

dkroenke Sep 3, 2020

bishibashiB Sep 3, 2020


		#include "iceoryx_utils/internal/cxx/forward_list.inl"

		#endif // IOX_UTILS_CXX_FORWARD_LIST_HPP

iox-#186 Add first cxx::forward_list implementation to utils #189

iox-#186 Add first cxx::forward_list implementation to utils #189

Conversation

mossmaurice commented Jul 13, 2020

elfenpiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bishibashiB Jul 15, 2020 • edited

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

bishibashiB commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bishibashiB commented Aug 14, 2020

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

bishibashiB Jul 15, 2020 •

edited