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

Better handling of internal failure modes and fix section bug #81

Merged
merged 24 commits into from
Apr 13, 2023
Merged

Conversation

cschreib
Copy link
Member

@cschreib cschreib commented Apr 10, 2023

This PR does the following:

  • Add guidelines for contributions.
  • Refactor snitch::small_function to stronger invariants (always contains a function to call). This is technically a breaking change (the class lost its empty() member, and is no longer default-constructible), but this should not affect anyone using snitch as a testing framework.
  • Refactor the default reporter to use the standard reporter interface, rather than special treatment.
  • Replace several calls to std::terminate to a customizable assertion handler (terminates by default, but can be made to throw instead for testing).
  • Apply the Lakos Rule for noexcept to the whole code. This led to a minor but measurable performance degradation (on compilation time mostly).
  • Add tests for various failure modes which used to unconditionally terminate.
  • Fixes Default reporter: line jump may be missed if failure message exceeds maximum message length  #82.
  • Fixes Nested sections don't always execute #83.

@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

Merging #81 (2800a42) into main (d6cb28d) will increase coverage by 0.68%.
The diff coverage is 92.61%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   92.61%   93.29%   +0.68%     
==========================================
  Files           2        2              
  Lines        1272     1237      -35     
==========================================
- Hits         1178     1154      -24     
+ Misses         94       83      -11     
Impacted Files Coverage Δ
src/snitch.cpp 89.54% <87.85%> (-1.14%) ⬇️
include/snitch/snitch.hpp 97.57% <97.91%> (+2.53%) ⬆️

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 f3a6398...2800a42. Read the comment docs.

- Functions in `<algorithm>` ([except `std::stable_sort`, `std::stable_partition`, and `std::inplace_merge`](https://stackoverflow.com/a/46714875/1565581)).
- Concepts and type traits.

Any type or function not listed above *should* be assumed to use heap memory unless demonstrated otherwise. One way to monitor this is on Linux is to use [valgrind/massif](https://valgrind.org/docs/manual/ms-manual.html).
Copy link
Member

Choose a reason for hiding this comment

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

-One way to monitor this is on Linux is to use
+One way to monitor this on Linux is to use

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot, thanks!


Therefore:
- Place as much code as possible in the `*.cpp` files rather than in headers.
- When not possible (templates, constexpr, etc.), consider if you can use a short and clear hand-written alternative instead. For example, `std::max(a, b)` requires `<algorithm>`, but can also be written as `a > b ? a : b`. Some of the simplest algorithms in `<algorithm>`, like `std::copy`, can also be written with an explicit loop.
Copy link
Member

Choose a reason for hiding this comment

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

-and clear hand-written alternative instead
+and clear handwritten alternative instead

ngrams

Copy link
Member Author

Choose a reason for hiding this comment

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

The more you know :) Thanks

@cschreib cschreib changed the title Better handling of internal failure modes Better handling of internal failure modes and fix section bug Apr 13, 2023
@cschreib cschreib merged commit 28771f7 into main Apr 13, 2023
@cschreib cschreib deleted the cleanup branch April 13, 2023 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants