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

Remove shared reporter state (+add inplace_any and type_id) #172

Merged
merged 3 commits into from
May 27, 2024

Conversation

cschreib
Copy link
Member

@cschreib cschreib commented May 19, 2024

This PR removes the global state for class-based reporter (see, e.g., console or xml reporters). Before this PR, calling REGISTER_REPORTER("name", Type); would create a global std::optional<Type>, which would be assigned whenever the reporter is requested. This works, but means there can only be a single instance of the reporter at any given time. Although this should be OK for 99% of use cases, this is an unnecessary limitation. The registry class should own the reporter instance.

The difficulty is we don't know what type the reporter can have, as this is an open ended set (user can add new ones we don't know about). This is normally solved by using polymorphism and std::unique_ptr<BaseType>, but we cannot do this as it requires an allocation. Here, this is instead achieved using a type-erased inplace storage, inplace_any, equivalent of std::any but without any allocation. This storage is used to manage the reporter instance, which is created on demand and destroyed when no longer used.

@cschreib cschreib force-pushed the cschreib/reporter-state branch 2 times, most recently from 3157b26 to 132480b Compare May 19, 2024 19:15
Copy link

codecov bot commented May 19, 2024

Codecov Report

Attention: Patch coverage is 96.72131% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 94.00%. Comparing base (f324947) to head (add06dc).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   93.88%   94.00%   +0.11%     
==========================================
  Files          28       29       +1     
  Lines        1651     1700      +49     
==========================================
+ Hits         1550     1598      +48     
- Misses        101      102       +1     
Files Coverage Δ
include/snitch/snitch_any.hpp 100.00% <100.00%> (ø)
include/snitch/snitch_registry.hpp 86.36% <100.00%> (+3.50%) ⬆️
src/snitch_registry.cpp 95.13% <100.00%> (ø)
src/snitch_reporter_console.cpp 97.14% <100.00%> (-0.08%) ⬇️
include/snitch/snitch_type_id.hpp 50.00% <50.00%> (ø)

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f324947...add06dc. Read the comment docs.

@cschreib cschreib force-pushed the cschreib/reporter-state branch 3 times, most recently from 392f2de to 3bcbea8 Compare May 19, 2024 20:46
@cschreib cschreib merged commit d663212 into main May 27, 2024
43 checks passed
@cschreib cschreib deleted the cschreib/reporter-state branch May 27, 2024 07:04
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.

2 participants