iox-#1614 Move uninitialized array #1774

FerdinandSpitzschnueffler · 2022-11-01T14:15:00Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This PR moves the UninitializedArray to iceoryx_hoofs/containers/include/iox/detail/ and removes the namespace containers.

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes implement uninitialized array #1614

codecov · 2022-11-01T14:39:03Z

Codecov Report

Merging #1774 (bc37c9b) into master (69ed96d) will decrease coverage by 0.03%.
The diff coverage is 91.17%.

@@            Coverage Diff             @@
##           master    #1774      +/-   ##
==========================================
- Coverage   77.39%   77.35%   -0.04%     
==========================================
  Files         367      367              
  Lines       14245    14243       -2     
  Branches     1992     1992              
==========================================
- Hits        11025    11018       -7     
- Misses       2591     2592       +1     
- Partials      629      633       +4

Flag	Coverage Δ
unittests	`77.01% <91.17%> (-0.04%)`	⬇️
unittests_timing	`15.64% <23.52%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../include/iceoryx_dust/posix_wrapper/named_pipe.hpp	`100.00% <ø> (ø)`
...ntainer/include/iox/detail/uninitialized_array.inl	`100.00% <ø> (ø)`
...oofs/container/include/iox/uninitialized_array.hpp	`100.00% <ø> (ø)`
iceoryx_hoofs/include/iceoryx_hoofs/cxx/stack.hpp	`100.00% <ø> (ø)`
iceoryx_hoofs/include/iceoryx_hoofs/cxx/string.hpp	`100.00% <ø> (ø)`
...ceoryx_hoofs/include/iceoryx_hoofs/cxx/variant.hpp	`100.00% <ø> (ø)`
iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp	`100.00% <ø> (ø)`
...include/iceoryx_hoofs/internal/concurrent/sofi.hpp	`100.00% <ø> (ø)`
...include/iceoryx_hoofs/posix_wrapper/posix_call.hpp	`100.00% <ø> (ø)`
...oryx_hoofs/source/posix_wrapper/access_control.cpp	`64.70% <ø> (ø)`
... and 7 more

elBoberido

Looks fine. Just one question related to the location and it seems bazel is not happy

doc/website/release-notes/iceoryx-unreleased.md

elBoberido · 2022-11-01T16:06:44Z

Oh, the clang-tidy warning will disappear once you rebase to master :)

…amespace iox Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

Move header file to iox folder, remove 's' from buffer and container modules Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

elBoberido

I think the clang-tidy-diff file could be simplified

.clang-tidy-diff-scans.txt

Replace UninitializedArrays with ElementType char with char array Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

Refactoring in UninitializedArray, variant and optional. Change gcc version to 8 for ubuntu build. Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

MatthiasKillat

The brace zero initialization question is the only relevant comment. Everything else is only a style question or potential future refactoring in a different issue.

iceoryx_hoofs/container/include/iox/uninitialized_array.hpp

MatthiasKillat · 2022-11-07T12:36:24Z

iceoryx_hoofs/container/include/iox/uninitialized_array.hpp

+    {
+        // AXIVION Next Construct AutosarC++19_03-A18.1.1 : required by low level UninitializedArray building block and encapsulated in abstraction
+        // NOLINTNEXTLINE(cppcoreguidelines-avoid-c-arrays, hicpp-avoid-c-arrays)
+        cxx::byte_t data[sizeof(ElementType)];


Adding any other fields to the struct will break things. Maybe worth a comment.

I am also wondering whether the zero-initialization braces { } are required here (instead of array of element_t).

MatthiasKillat · 2022-11-07T12:41:29Z

iceoryx_hoofs/container/include/iox/uninitialized_array.hpp

-    alignas(ElementType) element_t value[Capacity]{};
-    // NOLINTEND(cppcoreguidelines-avoid-c-arrays, hicpp-avoid-c-arrays)
+    // NOLINTNEXTLINE(cppcoreguidelines-avoid-c-arrays, hicpp-avoid-c-arrays)
+    element_t value[Capacity]{};


If we move the braces to element_t, they should not be required here. I am generally not sure how it works with padding without trying it out.

There can be trailing padding in element_t (to make size a multiple of alignment). Depends on our requirements of the padding initialization. Padding initialization which would not happen if we move it I think, but would it as it is now?

@elBoberido FYI

Since the UninitialiizedArray is typed and element_t uses the size and alignment of that type, there are no issues with additional padding bytes

MatthiasKillat · 2022-11-07T12:43:18Z

iceoryx_hoofs/include/iceoryx_hoofs/cxx/optional.hpp

@@ -229,7 +230,14 @@ class optional final : public FunctionalInterface<optional<T>, T, void>
    //     initHandle(&handle);
    //   }
    bool m_hasValue{false};
-    typename std::aligned_storage<sizeof(T), alignof(T)>::type m_data;
+    struct alignas(T) element_t


Should not be necessary to use our own struct. Do we want to remove the dependency on std::aligned_storage or is there another reason?

I replaced std::aligned_storage since it is deprecated in C++23 (I know, not a problem now but maybe later) and already added similar constructs in variant and UninitializedArray.

iceoryx_hoofs/include/iceoryx_hoofs/cxx/variant.hpp

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/variant.inl

elBoberido

Looks good. Just a few minor remarks

iceoryx_hoofs/include/iceoryx_hoofs/cxx/variant.hpp

elBoberido · 2022-11-07T13:18:40Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/variant.inl

@@ -31,7 +31,7 @@ inline constexpr variant<Types...>::variant(const variant& rhs) noexcept
 {
    if (m_type_index != INVALID_VARIANT_INDEX)
    {
-        internal::call_at_index<0, Types...>::copyConstructor(m_type_index, rhs.m_storage, m_storage);
+        internal::call_at_index<0, Types...>::copyConstructor(m_type_index, &rhs.m_storage.data[0], &m_storage.data[0]);


I think this leads to warnings with -Wcast-align=strict. Unfortunately this is baked into the call_at_index struct.

@FerdinandSpitzschnueffler after some thinking, the call_at_index should use void* instead of byte_t*. Could you please change that?

With that change there is only two offender when using -Wcast_aling=strict

One is in types.h in the C binding. Changing

/// @brief handle of the chunk header typedef struct { // could be empty but then we get `struct has no members` warning uint8_t do_not_touch_me[1]; } iox_chunk_header_t;

to

/// @brief handle of the chunk header typedef struct iox_chunk_header_opaque_t iox_chunk_header_t;

fixes the problem there.

And the others in the UDS and MQ iceperf code ... which is just by coincidence not broken 😅

I think types.h and the iceperf example could be fixed in another PR.

Also in another issue, in types.h we may consider defining iox::byte to be char and not uint8_t. This is mainly to prevent some issues with strict aliasing with compilers that do not define uint8_t to be unsigned char (I do not know of such a compiler).

It is mandated by the standard that no type can be smaller than char (and unsigned char I guess), and sizeof(char) = 1. So we do not lose guarantees but prevent some strict aliasing problems on exotic compilers.

To my knowledge, there is no reason to define a byte not to be char instead of uint8_t. Note that C++17 offers std::byte but we cannot use that here, see https://stackoverflow.com/questions/46150738/how-to-use-new-stdbyte-type-in-places-where-old-style-unsigned-char-is-needed.

I changed call_at_index to take a void* and created #1778 for the warnings in the C binding and the iceperf example.

@MatthiasKillat I like the idea of a iox::byte similar to std::byte. Would be something for a separate issue.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/variant.inl

…ve .data Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

elBoberido

My last remark can also be done in a separate PR since it is not really related to the uninitialized array

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/variant_internal.hpp

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

FerdinandSpitzschnueffler added refactoring Refactor code without adding features globex labels Nov 1, 2022

FerdinandSpitzschnueffler self-assigned this Nov 1, 2022

elfenpiff previously approved these changes Nov 1, 2022

View reviewed changes

elBoberido reviewed Nov 1, 2022

View reviewed changes

doc/website/release-notes/iceoryx-unreleased.md Outdated Show resolved Hide resolved

FerdinandSpitzschnueffler dismissed elfenpiff’s stale review via 9d905aa November 1, 2022 16:09

FerdinandSpitzschnueffler force-pushed the iox-1614-move-uninitialized-array branch from 4c157ea to 9d905aa Compare November 1, 2022 16:09

iox-eclipse-iceoryx#1614 Move uninitialized array to detail folder, n…

edd3f99

…amespace iox Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

FerdinandSpitzschnueffler force-pushed the iox-1614-move-uninitialized-array branch from 9d905aa to edd3f99 Compare November 1, 2022 16:11

iox-eclipse-iceoryx#1614 Move files

c6e8967

Move header file to iox folder, remove 's' from buffer and container modules Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

elBoberido previously approved these changes Nov 2, 2022

View reviewed changes

.clang-tidy-diff-scans.txt Outdated Show resolved Hide resolved

elfenpiff previously approved these changes Nov 2, 2022

View reviewed changes

iox-eclipse-iceoryx#1614 Revert some changes, adapt clang tidy file

e988034

Replace UninitializedArrays with ElementType char with char array Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

FerdinandSpitzschnueffler dismissed stale reviews from elfenpiff and elBoberido via e988034 November 3, 2022 19:30

elBoberido previously approved these changes Nov 3, 2022

View reviewed changes

FerdinandSpitzschnueffler dismissed elBoberido’s stale review via 042878c November 4, 2022 13:07

FerdinandSpitzschnueffler force-pushed the iox-1614-move-uninitialized-array branch from 042878c to 6cb1a0f Compare November 4, 2022 13:19

iox-eclipse-iceoryx#1614 Use struct for array alignment

c31b000

Refactoring in UninitializedArray, variant and optional. Change gcc version to 8 for ubuntu build. Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

FerdinandSpitzschnueffler force-pushed the iox-1614-move-uninitialized-array branch from 6cb1a0f to c31b000 Compare November 4, 2022 13:47

FerdinandSpitzschnueffler requested review from elBoberido and elfenpiff November 4, 2022 17:19

MatthiasKillat previously approved these changes Nov 7, 2022

View reviewed changes

elBoberido reviewed Nov 7, 2022

View reviewed changes

iox-eclipse-iceoryx#1614 Change byte* in call_at_index to void*, remo…

200616c

…ve .data Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

FerdinandSpitzschnueffler dismissed MatthiasKillat’s stale review via 200616c November 7, 2022 15:26

FerdinandSpitzschnueffler mentioned this pull request Nov 7, 2022

Fix warnings with -Wcast-align=strict #1778

Open

FerdinandSpitzschnueffler requested a review from elBoberido November 7, 2022 15:43

FerdinandSpitzschnueffler requested a review from MatthiasKillat November 7, 2022 15:43

elBoberido previously approved these changes Nov 7, 2022

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/variant_internal.hpp Outdated Show resolved Hide resolved

iox-eclipse-iceoryx#1614 Const correctness in variant_internal.hpp

bc37c9b

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

FerdinandSpitzschnueffler dismissed elBoberido’s stale review via bc37c9b November 7, 2022 18:32

elBoberido approved these changes Nov 7, 2022

View reviewed changes

elfenpiff approved these changes Nov 8, 2022

View reviewed changes

FerdinandSpitzschnueffler merged commit 6a58416 into eclipse-iceoryx:master Nov 8, 2022

FerdinandSpitzschnueffler deleted the iox-1614-move-uninitialized-array branch November 8, 2022 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#1614 Move uninitialized array #1774

iox-#1614 Move uninitialized array #1774

FerdinandSpitzschnueffler commented Nov 1, 2022 •

edited

Loading

codecov bot commented Nov 1, 2022 •

edited

Loading

elBoberido left a comment

elBoberido commented Nov 1, 2022

elBoberido left a comment

MatthiasKillat left a comment

MatthiasKillat Nov 7, 2022

MatthiasKillat Nov 7, 2022

elBoberido Nov 7, 2022

MatthiasKillat Nov 7, 2022

FerdinandSpitzschnueffler Nov 7, 2022

elBoberido left a comment

elBoberido Nov 7, 2022

elBoberido Nov 7, 2022 •

edited

Loading

MatthiasKillat Nov 7, 2022 •

edited

Loading

FerdinandSpitzschnueffler Nov 7, 2022

elBoberido Nov 7, 2022

elBoberido left a comment

iox-#1614 Move uninitialized array #1774

iox-#1614 Move uninitialized array #1774

Conversation

FerdinandSpitzschnueffler commented Nov 1, 2022 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

codecov bot commented Nov 1, 2022 • edited Loading

Codecov Report

elBoberido left a comment

Choose a reason for hiding this comment

elBoberido commented Nov 1, 2022

elBoberido left a comment

Choose a reason for hiding this comment

MatthiasKillat 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

elBoberido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

MatthiasKillat Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

FerdinandSpitzschnueffler commented Nov 1, 2022 •

edited

Loading

codecov bot commented Nov 1, 2022 •

edited

Loading

elBoberido Nov 7, 2022 •

edited

Loading

MatthiasKillat Nov 7, 2022 •

edited

Loading