search

snitch-org / snitch

Lightweight C++20 testing framework.
Boost Software License 1.0
252 stars 7 forks source link

feature switch for thread_local variables and multi-threading #159

Closed CrustyAuklet closed 3 months ago

CrustyAuklet commented 3 months ago

thread local variables will trigger a memory allocation on a bare metal system. When running tests on a single thread, or a system without threads, there is no benefit to using a thread_local variable. As discussed in #158.

This change creates a feature flag to control if snitch should be compiled with support for threading. A macro SNITCH_THREAD_LOCAL is defined in the config header that resolves to nothing when the feature flag is disabled, or thread_local when it is enabled. The feature flag is enabled by default to preserve existing behavior.

I names the feature flag SNITCH_WITH_MULTITHREADING to let it be used for any future threading features as well. If you want a more granular option that is an easy change.

I updated the CMake project file, and the meson options. I don't use meson so someone double checking my work there would be great. The change seems simple enough though 😄

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.87%. Comparing base (fb1f8dd) to head (73f4935).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/snitch-org/snitch/pull/159/graphs/tree.svg?width=650&height=150&src=pr&token=X422DE81PN&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org)](https://app.codecov.io/gh/snitch-org/snitch/pull/159?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org) ```diff @@ Coverage Diff @@ ## main #159 +/- ## ======================================= Coverage 93.87% 93.87% ======================================= Files 27 27 Lines 1664 1664 ======================================= Hits 1562 1562 Misses 102 102 ``` | [Files](https://app.codecov.io/gh/snitch-org/snitch/pull/159?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org) | Coverage Δ | | |---|---|---| | [src/snitch\_test\_data.cpp](https://app.codecov.io/gh/snitch-org/snitch/pull/159?src=pr&el=tree&filepath=src%2Fsnitch_test_data.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org#diff-c3JjL3NuaXRjaF90ZXN0X2RhdGEuY3Bw) | `95.45% <ø> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/snitch-org/snitch/pull/159?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/snitch-org/snitch/pull/159?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org). Last update [fb1f8dd...73f4935](https://app.codecov.io/gh/snitch-org/snitch/pull/159?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org).
cschreib commented 3 months ago

The CI failures weren't related to this PR and have been fixed in main. Once you have addressed the review comments above; please rebase onto the latest main, or merge main. This should make the build go green.