Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added type_id() & type_size() methods to the cetl::unbounded_variant. #verification #docs #sonar #127

Merged
merged 14 commits into from
Jun 10, 2024

Conversation

serges147
Copy link
Contributor

No description provided.

@serges147 serges147 requested a review from pavel-kirienko June 5, 2024 13:50
Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

After the special case is removed it should be good to go

cetlvast/suites/unittest/test_unbounded_variant.cpp Outdated Show resolved Hide resolved
include/cetl/rtti.hpp Outdated Show resolved Hide resolved
include/cetl/rtti.hpp Outdated Show resolved Hide resolved
@serges147 serges147 self-assigned this Jun 5, 2024
pavel-kirienko
pavel-kirienko previously approved these changes Jun 5, 2024
@serges147 serges147 requested a review from thirtytwobits June 5, 2024 16:12
Base automatically changed from sshirokov/84_pf to main June 5, 2024 17:24
@serges147 serges147 dismissed pavel-kirienko’s stale review June 5, 2024 17:24

The base branch was changed.

pavel-kirienko
pavel-kirienko previously approved these changes Jun 5, 2024
value_const_converter_ = [](const void* const storage, const type_id& id) {
const auto ptr = static_cast<const Tp*>(storage);
return ptr->_cast_(id);
value_const_converter_ = [](const void* const storage, const cetl::type_id& dst_type_id) {
Copy link
Member

Choose a reason for hiding this comment

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

These lambdas may fail on systems with no heap and a deficient small-value-optimization in std::function. Please use function pointers to avoid this.

Copy link
Contributor Author

@serges147 serges147 Jun 6, 2024

Choose a reason for hiding this comment

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

I don't think so, they are just pure (implicitly generated) static functions, they don't capture anything (neither any of outside vars nor this), only Tp type information - so no allocation. sizeof of them is 1 (should kinda be zero but as you know even empty struct sizeof is 1). And the fact that I can just assign them to a simple pointer to function (see value_const_converter_ declaration below) proves it - as soon as add anything to the capture list such assignments won't compile anymore.

Here is sizeof of this lambda:

        using Xxx = decltype([](const void* const storage, const cetl::type_id& dst_type_id) {
            CETL_DEBUG_ASSERT(nullptr != storage, "");
            const auto        ptr     = static_cast<const Tp*>(storage);
            const void* const dst_ptr = ptr->_cast_(dst_type_id);
            return std::make_pair(dst_ptr, cetl::type_id_value<Tp>);
        });
        static_assert(sizeof(Xxx) == 1);

Copy link
Contributor Author

@serges147 serges147 Jun 6, 2024

Choose a reason for hiding this comment

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

In addition, although I could technically make explicit template static member functions for convertors, I'm reluctant todo so b/c:

  1. There will be 4 of them (see two mutually exclusive versions of make_converters).
  2. They all will be out of make_converters context.
  3. make_converters is the only supposed entity to reference them; and of course nobody is allowed to call them directly (only indirectly via value_[mut|const]_convertor_ function pointers). So, in some sense I believe am doing proper isolation here without introducing any indeterministic behavior (including memory allocations).
  4. We also have other function pointer members initialized with lambdas in a similar manner (and isolation!), like value_destroyer_, value_copier_ and value_mover_.
  5. Debugging is not affected - breakpoints or stepping into such lambdas perfectly works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Furthermore, I scanned AUTOSAR14 spec, and found no lambda rule against my implementation. The only remotely applicable rule is the following:

Rule A5-1-6 (advisory, implementation, automated)
Return type of a non-void return type lambda expression should be
explicitly specified.

I can fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done - see commit

Copy link
Member

Choose a reason for hiding this comment

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

To back up what Sergei said:

image

pavel-kirienko
pavel-kirienko previously approved these changes Jun 6, 2024
@serges147 serges147 requested a review from pavel-kirienko June 6, 2024 11:44
pavel-kirienko
pavel-kirienko previously approved these changes Jun 6, 2024
include/cetl/rtti.hpp Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Jun 6, 2024

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@serges147 serges147 requested a review from pavel-kirienko June 6, 2024 12:47
Copy link
Member

@thirtytwobits thirtytwobits left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks.

@serges147 serges147 merged commit 5aa0a5b into main Jun 10, 2024
33 of 34 checks passed
@serges147 serges147 deleted the sshirokov/type_id branch June 10, 2024 08:57
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.

3 participants