PMR support for `cetl::unbounded_variant` & `cetl::pmr::function`. by serges147 · Pull Request #124 · OpenCyphal/CETL

serges147 · 2024-05-28T08:07:49Z

Added IsPmr support to cetl::unbounded_variant.
Added new cetl::pmr::function (aka std::function but with PMR instead of c++ heap).
Added new cetl::pmr::InterfacePtr and its InterfaceFactrory::make_unique
- based on std::unique_ptr but with custom PMR-based allocator and deleter
- allows to hide implementation (aka Concrete) type from result InterfacePtr
- allows to delete destructor from an interface type

…#sonar

Node.js 16 actions are deprecated. Please update the following actions to use Node.js 20: actions/cache@v3

…ation #docs #sonar

thirtytwobits

We also have a cetlpf.hpp in unbounded_variant.hpp we need to remove.

Will use of "cetl/unbounded_variant.hpp" compile if IsPmr is false and no pmr types are included? I would expect this. That is, if we don't require PMR then we should require inclusion of PMR types.

thirtytwobits · 2024-05-28T16:54:07Z

+#    include "cetl/cetl.hpp"
+#endif
+
+#include "cetl/pf17/cetlpf.hpp"


Don't include polyfill headers in primary hpp files. The use of any cetlpf.hpp is optional.

thirtytwobits · 2024-05-28T17:00:14Z

+        //
+        value_size_ = size_bytes;
+
+        return inplace_buffer_;


should we not check that size_bytes is <= Footprint and return null if not?

No, for non-pmr there is no such thing as "null" storage. Please see static assert at make_handlers - this method is one of the core entry points when it come to creation of a new unbounded variable. So it won't compile (and there is compile test to ensure this, see test_unbounded_variant_footprint_set.cpp).

There are also two other static asserts on attempt to read an arbitrary value (see get_ptr overloads, and corresponding test_unbounded_variant_footprint_get_[non_]const.cpp compile tests).

serges147 · 2024-05-29T11:54:22Z

We also have a cetlpf.hpp in unbounded_variant.hpp we need to remove.

Sorry Scott, but when it comes to "include or not to include" of these two...

cetl/pfNN/cetlpf.hpp
#ifndef CETL_H_ERASE #include "cetl/cetl.hpp" #endif

... I'm quite confused when which... so far I tried to do as it's in other files.
Also, at cetl/unbounded_variant.hpp I use cetl::byte (which is either std::byte or our cetl::pf17::byte depending on c++ standard), so how can I use cetl::byte without including first cetl/pf17/cetlpf.hpp ?

Will use of "cetl/unbounded_variant.hpp" compile if IsPmr is false and no pmr types are included? I would expect this. That is, if we don't require PMR then we should require inclusion of PMR types.

pmr types are included regardless of IsPmr. I believe it's inevitable (IsPmr is template param, not like some #define). Having said this, You don't pay for what you don't use - all these PMR-specific methods should not be instantiated in the first place (and there are static_assert-s to ensure you don't call them by mistake).

…erface deleter. "just in case" b/c `std::unique_ptr` is not allowing invalid casting in the first place, but I believe such change makes it more explicit (and it was asked for in the first draft review).

…4_pmr_fun_ubvar_/valueless_2

thirtytwobits · 2024-05-29T14:07:12Z

Sorry Scott, but when it comes to "include or not to include" of these two...

cetl/pfNN/cetlpf.hpp

#ifndef CETL_H_ERASE #include "cetl/cetl.hpp" #endif

... I'm quite confused when which... so far I tried to do as it's in other files. Also, at cetl/unbounded_variant.hpp I use cetl::byte (which is either std::byte or our cetl::pf17::byte depending on c++ standard), so how can I use cetl::byte without including first cetl/pf17/cetlpf.hpp ?

The polyfill headers are completely optional in CETL so nothing in CETL should include them, they should include other things from CETL. As for byte, you'll just have to use uint8_t internally. I don't think we're providing byte as part of a public API anywhere in here.

The ideal CETL type only includes cetl.hpp but there are some exceptions to this rule; just never polyfill headers is all. One thing you'll note is the pf headers have a lot of AUTOSAR violations. It's assumed that users of the polyfill are more interested in convenience and portability then pedantry whereas high-criticality code will avoid these headers using the CETL types directly.

thirtytwobits · 2024-05-29T14:11:59Z

pmr types are included regardless of IsPmr. I believe it's inevitable (IsPmr is template param, not like some #define). Having said this, You don't pay for what you don't use - all these PMR-specific methods should not be instantiated in the first place (and there are static_assert-s to ensure you don't call them by mistake).

If PMR types are always required then we shouldn't also need the "isPMR" parameter. Instead we should just model the default storage as a memory_resource we define locally.

…4_pmr_fun_ubvar_/valueless_fun #verification #docs #sonar

pavel-kirienko

As we discussed, it is desirable to eliminate the use of PMR from the deleter, or at least (this may be a better option) provide a new non-PMR deleter that does not have the up-converting constructor and which is used by default while allowing the user to switch to the other one in case of great necessity.

pavel-kirienko · 2024-06-03T09:09:15Z

+/// and `reset` (or `reset(Pmr*)`) method to recover from it.
+///
+template <typename Result, typename... Args, std::size_t Footprint, typename Pmr = void>
+inline void swap(cetl::pmr::function<Result(Args...), Footprint, Pmr>& lhs,


inline is redundant for function templates, they are inline by default.

pavel-kirienko · 2024-06-03T09:13:24Z

+/// and depending on which stage of swapping the failure happened
+/// it could affect (invalidate) either of `lhs` or `rhs` function.
+/// Use `valueless_by_exception()` method to check if a function is in such failure state,
+/// and `reset` (or `reset(Pmr*)`) method to recover from it.


Suggested change

/// and `reset` (or `reset(Pmr*)`) method to recover from it.

/// and `reset` (or `reset(Pmr*)`) method (or assign a new value) to recover from it.

pavel-kirienko · 2024-06-03T09:14:26Z

+        other.handler_ptr_ = get_if<handler_t>(&other.any_handler_);
+    }
+
+    /// True if the function is valueless b/c of an exception.


Suggested change

/// True if the function is valueless b/c of an exception.

/// True if the function is valueless b/c of an exception or OOM.

OOM doesn't necessarily cause an exception

pavel-kirienko · 2024-06-03T09:15:18Z

+        return (base::get_value_size() > 0UL) && (nullptr != value_destroyer_);
+    }
+
+    /// True if the variant is valueless b/c of an exception.


Suggested change

/// True if the variant is valueless b/c of an exception.

/// True if the variant is valueless b/c of an exception or OOM.

sonarqubecloud · 2024-06-03T09:47:11Z

Quality Gate failed

Failed conditions
88.5% Coverage on New Code (required ≥ 90%)

See analysis details on SonarCloud

serges147 added 30 commits May 14, 2024 17:15

backup

eaa64d7

replace cetl::unbounded_variant with std::function #verification …

2f93a7a

…#sonar

#verification

386c1c0

actions/cache@v3 → v4 #verification

b942686

Node.js 16 actions are deprecated. Please update the following actions to use Node.js 20: actions/cache@v3

minor fixes #verification #docs #sonar

ef1001f

first draft of cetl::pmr::function

bd42ce4

InterfacePtr with cetl::pmr::function #verification #sonar

3beefb1

renamings

ae265d7

new_delete_resource → get_default_resource #verification #docs #sonar

a1d0fc6

Merge branch 'sshirokov/118_unique_ptr' into sshirokov/84_pmr_function

9535f49

#verification #sonar

db2f746

#verification #sonar

3375a03

#verification #sonar

2a8b516

#verification #sonar

69c8bc6

minor comment

b3eb6e6

draft of IsPmr ubvar

d90ae93

Implemented <Footprint=0, IsPrm=true>

2044f35

Implemented <Footprint=1+, IsPrm=true>

f095ad0

fix gcc compile

9931eaf

break function to 2 function_base partial specialization #verific…

54b1bde

…ation #docs #sonar

#verification #docs #sonar

8d15d94

added out of memory handling #verification #docs #sonar

c2b2814

#verification #docs #sonar

795a333

more coverage #verification #docs #sonar

edefe7b

#verification #sonar

de2f14f

backup

3928e08

#verification #docs #sonar

023d4f7

#verification #docs #sonar

3c84cb3

#verification #docs #sonar

157f844

#verification #docs #sonar

fcbbc61

serges147 marked this pull request as ready for review May 28, 2024 10:31

serges147 requested a review from lydia-at-amazon May 28, 2024 10:35

thirtytwobits reviewed May 28, 2024

View reviewed changes

serges147 added 2 commits May 29, 2024 12:42

#verification #docs #sonar

5d3023c

#verification #docs #sonar

a994e6c

serges147 added 2 commits May 29, 2024 15:23

Just in case adding is_base_of check for up-casting-ctor of our int…

dc36c60

…erface deleter. "just in case" b/c `std::unique_ptr` is not allowing invalid casting in the first place, but I believe such change makes it more explicit (and it was asked for in the first draft review).

Merge branch 'sshirokov/84_pmr_fun_ubvar_/valueless' into sshirokov/8…

f30c8a8

…4_pmr_fun_ubvar_/valueless_2

serges147 added 4 commits May 29, 2024 18:02

fixed functor decay issue; added docs; #verification #docs #sonar

a9de16b

get rid of usage of cetl::byte polyfill.

ba29ed5

IsPmr → Pmr #verification #docs #sonar

4052fce

Merge branch 'sshirokov/84_pmr_fun_ubvar_/valueless' into sshirokov/8…

9204f62

…4_pmr_fun_ubvar_/valueless_fun #verification #docs #sonar

pavel-kirienko reviewed May 31, 2024

View reviewed changes

Comment thread include/cetl/pmr/interface_ptr.hpp Outdated

pavel-kirienko previously approved these changes May 31, 2024

View reviewed changes

serges147 added 2 commits May 31, 2024 20:51

removal of Pmr from InterfacePtr Deleter #verification #docs #sonar

e468494

docs

5c97ea1

serges147 dismissed pavel-kirienko’s stale review via 5c97ea1 May 31, 2024 18:17

style fix

fa83949

pavel-kirienko previously approved these changes Jun 3, 2024

View reviewed changes

minor review fixes

8da693a

serges147 dismissed pavel-kirienko’s stale review via 8da693a June 3, 2024 09:34

serges147 requested review from pavel-kirienko and thirtytwobits June 3, 2024 09:50

pavel-kirienko approved these changes Jun 3, 2024

View reviewed changes

thirtytwobits approved these changes Jun 3, 2024

View reviewed changes

serges147 merged commit 6510714 into main Jun 3, 2024

serges147 deleted the sshirokov/84_pmr_fun_ubvar_/valueless branch June 3, 2024 15:42

	/// and `reset` (or `reset(Pmr*)`) method to recover from it.
	/// and `reset` (or `reset(Pmr*)`) method (or assign a new value) to recover from it.

	/// True if the function is valueless b/c of an exception.
	/// True if the function is valueless b/c of an exception or OOM.

	/// True if the variant is valueless b/c of an exception.
	/// True if the variant is valueless b/c of an exception or OOM.

Conversation

serges147 commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thirtytwobits left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thirtytwobits May 28, 2024

Choose a reason for hiding this comment

Uh oh!

thirtytwobits May 28, 2024

Choose a reason for hiding this comment

Uh oh!

serges147 May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serges147 commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thirtytwobits commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thirtytwobits commented May 29, 2024

Uh oh!

Uh oh!

pavel-kirienko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pavel-kirienko Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

pavel-kirienko Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

pavel-kirienko Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

pavel-kirienko Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jun 3, 2024

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serges147 commented May 28, 2024 •

edited

Loading

serges147 May 29, 2024 •

edited

Loading

serges147 commented May 29, 2024 •

edited

Loading

thirtytwobits commented May 29, 2024 •

edited

Loading