Skip to content

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

Merged
serges147 merged 14 commits into
mainfrom
sshirokov/type_id
Jun 10, 2024
Merged

serges147 merged 14 commits into
mainfrom
sshirokov/type_id

Conversation

@serges147

Copy link
Copy Markdown
Contributor

No description provided.

@serges147
serges147 requested a review from pavel-kirienko June 5, 2024 13:50
Comment thread include/cetl/unbounded_variant.hpp
Comment thread include/cetl/unbounded_variant.hpp

@pavel-kirienko pavel-kirienko left a comment

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.

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

Comment thread cetlvast/suites/unittest/test_unbounded_variant.cpp Outdated
Comment thread include/cetl/rtti.hpp Outdated
Comment thread include/cetl/rtti.hpp Outdated
@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
Comment thread include/cetl/unbounded_variant.hpp Outdated
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
Copy Markdown
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.

@serges147 serges147 Jun 6, 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.

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);

@serges147 serges147 Jun 6, 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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Comment thread include/cetl/rtti.hpp
@sonarqubecloud

sonarqubecloud Bot commented Jun 6, 2024

Copy link
Copy Markdown

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

@thirtytwobits thirtytwobits left a comment

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.

Looks good. Thanks.

@serges147
serges147 merged commit 5aa0a5b into main Jun 10, 2024
@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