Skip to content

CETL's cetl::any - #113

Merged
thirtytwobits merged 19 commits into
mainfrom
issue/83_any
Apr 30, 2024
Merged

thirtytwobits merged 19 commits into
mainfrom
issue/83_any

Conversation

@serges147

Copy link
Copy Markdown
Contributor

Fix for issue #83

Sergei Shirokov and others added 9 commits March 14, 2024 08:59
…TTI yet. (#102)

- Implemented most of constructors (close to `std::any`).
- Added implementation of 3 `operator=`-s.
- Added first 2 overloads of `any_cast`.
- Added `make_any` overloads.
- Implemented `has_value`, `emplace`, `swap` & `reset`.
- `cetl::bad_any_cast` now alias to `cetl::pf17::bad_any_cast `.

Also:
- Appended `is_in_place_type_v` and `is_in_place_index_v` constexpr-s.
- Applied clang format to `utility.hpp` header file.

Related to #83

---------

Co-authored-by: Sergei Shirokov <sergei@Sergeis-Mac-mini.local>
…tures. (#104)

- Added "value return" overloads of any_cast (#1️⃣, 2️⃣ & 3️⃣).
- Added std::initializer_list overloads for
  - ctor (#6️⃣)
  - `emplace` (#2️⃣)
  - `make_any` (#2️⃣)
- Added corresponding unit tests, plus more cases to increase coverage.

Notes for to be continued...
- `Align` param fixes will be in the next pr.
- Copyable/Movable (as well as elimination of various `const_cast`) will
be in the after next pr.

---------

Co-authored-by: Sergei Shirokov <sergei@Sergeis-Mac-mini.local>
Also:
- Fixed `sizeof` → `alignof`.
- Added `TestMovableOnly` helper, and assign operators.
- Reduced code duplication in tests (`TestCopyable` &
`TestCopyableAndMovable`).

---------

Co-authored-by: Sergei Shirokov <sergei@Sergeis-Mac-mini.local>
- Now `Copyable` and `Movable` constraints are respected.
- Now instead of single handler we have 3 (destroyer, copier, mover) -
allows to eliminate AUTOSAR `const_cast` violations.
- In unit tests:
  - more tests to cover combinations of `any` template params
  - switch from pointer based `any_cast` to value or reference based.

---------

Co-authored-by: Sergei Shirokov <sergei@Sergeis-Mac-mini.local>
Co-authored-by: Pavel Kirienko <pavel.kirienko@gmail.com>
- Now CETL's polymorphic RTTI casting is in use at
`cetl::detail::base_storage`.
- Extended unit tests `TestXxx` helper types with virtual `what()`
method - to test polymorphic RTTI casting.
- Added compile tests to verify several `static_assert`-s at `cetl::any`
(related to `Footprint` fitting).

Also:
- Reduced code duplication at unit tests `TestXxx` helper types.
- Fixed incorrect applying of `-fno-exceptions` to C flags - should be
C++ flags.
- Addressed several unit tests todo-s. No todo-s anymore at production
`cetl::any` code.
- Added verification that there will be no value moved out of source
`any` in case of bad cast attempt.

This change was also made, but decided to revert (for now) and save it
as patch (for future reference):
- Added new `CETLVAST_DISABLE_CPP_RTTI` configuration parameter. `OFF`
by default.

[CETLVAST_DISABLE_CPP_RTTI.patch](https://github.com/OpenCyphal/CETL/files/14792699/CETLVAST_DISABLE_CPP_RTTI.patch)

---------

Co-authored-by: Sergei Shirokov <sergei@Sergeis-Mac-mini.local>
Co-authored-by: Sergei Shirokov <sshirokov@malwarebytes.com>
- Extended corresponding unit test to cover this case.
- Renamed `uut` → `any` in all `test_any` unit tests.

---------

Co-authored-by: Sergei Shirokov <sergei@Sergeis-Mac-mini.local>
@serges147 serges147 self-assigned this Apr 2, 2024
@pavel-kirienko pavel-kirienko linked an issue Apr 2, 2024 that may be closed by this pull request
pavel-kirienko
pavel-kirienko previously approved these changes Apr 2, 2024
Comment thread include/cetl/pf17/any.hpp Outdated
serges147 and others added 2 commits April 3, 2024 11:46
Co-authored-by: Pavel Kirienko <pavel.kirienko@gmail.com>
@pavel-kirienko

Copy link
Copy Markdown
Member

@thirtytwobits this is waiting for review. The Sonar failure is due to the coverage check which we discussed at the call.

Comment thread cetlvast/suites/unittest/test_any.cpp Outdated
Comment thread include/cetl/any.hpp Outdated
Comment thread include/cetl/any.hpp

#include "rtti.hpp"
#include "pf17/cetlpf.hpp"
#include "pf17/utility.hpp"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

move utility.hpp include inside of the #ifndef CETL_H_ERASE section below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@thirtytwobits With my recent renaming (anyunbounded_variant) AND removing all related PF-any-stuff, is this comment (and 2 below) are still applicable? I'm asking b/c I'm not sure what needs to be done...

Comment thread include/cetl/any.hpp
#define CETL_ANY_HPP_INCLUDED

#include "rtti.hpp"
#include "pf17/cetlpf.hpp"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

illegal dependency: CETL pf can use CETL first-class but not the other way around.

Comment thread include/cetl/any.hpp
#ifndef CETL_ANY_HPP_INCLUDED
#define CETL_ANY_HPP_INCLUDED

#include "rtti.hpp"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

move within CETL_H_ERASE

Comment thread include/cetl/pf17/any.hpp Outdated
/// Copyright Amazon.com Inc. or its affiliates.
/// SPDX-License-Identifier: MIT

#ifndef CETL_PF17_ANY_HPP_INCLUDED

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just omit this header for now. It's completely off the mark. What we actually want is to have something like this in the cetlpf.hpp

using bad_any_cast = std::bad_any_cast;

template <typename T> constexpr bool is_rtti_convertible = std::false_type::value;
using type_id = std::type_info;

// Forward declarations
template <std::size_t Footprint, bool Copyable, bool Movable, std::size_t Alignment> class any;

template <typename T> inline constexpr const type_id &type_id_value()
{
    return typeid(T);
}

template <typename T> using in_place_type_t = std::in_place_type_t<T>;

template <typename T> inline constexpr std::in_place_type_t<T> in_place_type;

template <std::size_t I> using in_place_index_t = std::in_place_index_t<I>;

template <std::size_t I> inline constexpr std::in_place_index_t<I> in_place_index;

// Forward declarations
template <std::size_t Footprint, bool Copyable, bool Movable, std::size_t Alignment> class any;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is easy to change but I want to confirm a few things.

  1. I think we originally discussed that the reliance on the native RTTI is not permitted, but now we're making it optional, configurable at build time, such that it conditionally replaces our cetl_rtti.

  2. Our any is not a drop-in replacement for std::any (unlike, say, variant or span), so introducing it into the polyfill header as-is seems questionable. The original intention was to introduce a particular instantiation of our any into the polyfill namespace, that instantiation being a better approximation of std::any. Alternatively, we can avoid polyfilling std::any completely considering the differences. Substituting our any template for std::any is unlikely to be what we want.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With a few tweaks we leave open the possibility that our implementation can be redefined to use RTTI. This seems worth the minor effort to maintain. As for inclusion in the polyfill, let's just leave that for a later time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Understood.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No more such header.

Comment thread include/cetl/any.hpp Outdated
/// where let `U` be `std::remove_cv_t<std::remove_reference_t<ValueType>>`.
///
template <typename ValueType, typename Any>
CETL_NODISCARD ValueType any_cast(const Any& operand)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should refuse to generate any_cast for non-pointer types when exceptions are disabled. This is the approach we took with variable_length_array.hpp

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm expecting that in user code it will result in this:

T& val = any_cast<T>(my_any);

Being replaced with this:

T* const val = any_cast<T*>(&my_any);
assert(val != nullptr);

Where there is a robust assumption that the any contains T. One example would be the DynamicBuffer in LibCyphal.

Is it done to uphold compliance with a coding standard or there is other rationale?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Of my two rules for API design:

  1. Make the API easy to use correctly
  2. Make the API hard to use incorrectly

rule 2 applies here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay. But for the record we have throwing overloads in cetl::variant, like cetl::get.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I renamed any_cast to two functions:

  • get for value-returning; throws cetl::bad_unbounding_variant_access in case of failure
  • get_if for pointer-returning; returns nullptr in case of failure
    So now, this api is close to usual (type-bound) cetl::variant.

Comment thread include/cetl/any.hpp
void make_converters() noexcept
{
value_const_converter_ = [](const void* const storage, const type_id& id) {
return (id == type_id_value<Tp>) ? storage : nullptr;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need to change type_id_value to be functional to allow for typeid's required runtime. That is, the token must be replaceable with typeid when CETL RTTI is erased. Here's an example polyfill:

template <typename T> inline constexpr const type_id &type_id_value()
{
    return typeid(T);
}

This would require the token to look like type_id_value<Tp>() so we can invoke typeid.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@thirtytwobits Scott, is it possible that we handle this request as a separate issue? I'm asking b/c I currently don't understand requirements, as well as the exact way to reproduce it.

@pavel-kirienko pavel-kirienko Apr 29, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@serges147 this is simply about replacing this:

template <typename TypeIDType>
constexpr type_id type_id_type_value = detail::type_id_type_value_impl(TypeIDType{});

with this:

template <typename TypeIDType>
constexpr type_id type_id_type_value() noexcept
{
    return detail::type_id_type_value_impl(TypeIDType{});
}

We must return by value because constexpr functions do not allow static storage duration locals. Alternatively, we keep the original type_id_type_value under a different name (perhaps move it into detail) and return a reference to it:

template <typename TypeIDType>
constexpr const type_id& type_id_type_value() noexcept
{
    return detail::type_id_type_value<TypeIDType>;
}

@serges147 serges147 Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, I did the replacement. unfortunately, I still don't understand what exactly was the problem (and how to reproduce it, f.e. in unit tests), but I did as you proposed

Comment thread include/cetl/any.hpp Outdated

// Forward declarations
template <std::size_t Footprint, bool Copyable, bool Movable, std::size_t Alignment>
class any;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is misleading. Let's rename the header and type to static_any to avoid any confusion that this is a different implementation then the standard requires.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

static may not be a good choice if we extend this any with PMR support which we discussed in the past. Perhaps the following are better options:

  • any2
  • configurable_any
  • heapless_any (works for PMR as well because we don't use the global heap directly)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • super_mega_any
  • this_any_sponsored_by_progressive__progressive_will_save_you_hundreds_on_car_insurance
  • less_evil_any

serious ones

many - sorta cheeky

any_of - what if we changed the template to take a variadic list of types and it does the max(sizeof(...)) so the usage would look like any_of<int, float, double> a{1};?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok here's another inspired by your comment:

  • unbounded_variant

what if we changed the template to take a variadic list of types and it does the max(sizeof(...)) so the usage would look like any_of<int, float, double> a{1};?

If you need variant, you know where to find it. Perhaps you want an overload for make_any; we could add that later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unbounded_variant. love it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cetl::unbounded_variant it is; no PF stuff anymore.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@thirtytwobits
thirtytwobits merged commit 10fbb2b into main Apr 30, 2024
@thirtytwobits
thirtytwobits deleted the issue/83_any branch April 30, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide PMR-aware cetl::any

3 participants