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

add runtime TIMES #333

Merged
merged 16 commits into from
Oct 6, 2024
Merged

add runtime TIMES #333

merged 16 commits into from
Oct 6, 2024

Conversation

DNKpp
Copy link
Contributor

@DNKpp DNKpp commented Apr 14, 2024

as discussed in this issue #283 it would be very helpful to have the TIMES action configurable with runtime arguments.
This PR aims to add that, without making too much changes to existing symbols (in fact, no changes are made at all).

As this is mainly a sketchup for the actual feature, I'm open for feedback regarding behavior and naming (maybe ``RT_TIMES is a more appropriate name).

@DNKpp DNKpp changed the title [wip] add runtime times configuration [wip] add runtime TIMES Apr 14, 2024
Copy link

codecov bot commented Apr 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.20%. Comparing base (746ea0e) to head (c77789f).
Report is 30 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #333   +/-   ##
=======================================
  Coverage   99.20%   99.20%           
=======================================
  Files          12       12           
  Lines        1003     1011    +8     
  Branches       22       22           
=======================================
+ Hits          995     1003    +8     
  Misses          8        8           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DNKpp
Copy link
Contributor Author

DNKpp commented Apr 14, 2024

These detected memory leaks seem to be caused by the exception thrown from the dynamic_times constructor. But that's not the root cause of the issue, as trompeloeil is surprisingly very bad at managing some of its resources (e.g. the matcher is "forwarded" via raw pointer).

So, that is rather unrelated to that PR here, but we should definitely address that in another PR.

@rollbear
Copy link
Owner

From a very brief glance, this looks interesting. I'm away on a conference right now, but I'll have a closer look when I get back home again.

@rollbear
Copy link
Owner

This is looking really neat. Thank you. Is there something you want to discuss, given that it's still marked [WIP]?

Otherwise, I think it's just a matter of adding docs and negative test(s) under 'compilation_errors' before I can merge it.

Naming can always be discussed, but I think DYN_TIMES is as good as anything I can come up with.

@rollbear
Copy link
Owner

Ping! I know I haven't been very attentive lately, but I hope to be able to tag a release in the tear future, say during the weekend July 13-14th. If there are things you want from me to be able to merge this, then let me know. As mentioned before, I'm cool with the change and the names, but would appreciate docs and some more tests.

@DNKpp
Copy link
Contributor Author

DNKpp commented Jul 1, 2024

Hey, sry for the delay. I'm currently very busy with my thesis, so I won't have much spare time for this during the next month or so.

I'm not sure, which tests I should add, as I mostly reuse existing components and already added a couple of tests. The negative tests are in the section beginning at 4835. Sure, we could explicitly add some constexpr tests, which will not compile with invalid bounds (because of the exception), (no, we can not, as c++11 constexpr is very limited) but I don't think that would be of much value.

I would really like to rename it to RT_TIMES as this seems to me to be more in line with the rest of the framework (which already uses the 2 sign prefixes for other features).

@rollbear
Copy link
Owner

rollbear commented Jul 3, 2024

Ok, no worries. There's no hurry.

@DNKpp
Copy link
Contributor Author

DNKpp commented Sep 26, 2024

Hey, I finally got the time to invest some time into this feature.
I renamed it to RT_TIMES and added some compilation error files. Are they ok?

@DNKpp DNKpp changed the title [wip] add runtime TIMES add runtime TIMES Sep 26, 2024
@DNKpp
Copy link
Contributor Author

DNKpp commented Oct 1, 2024

So, I think I'm done. Added the necessary documentation and checked, that all anchors are working as expected. Don't know why codecov now started complaining.

@rollbear
Copy link
Owner

rollbear commented Oct 1, 2024

Never mind codecov. It's being silly. I'll have a look. Thanks!

@rollbear rollbear merged commit 74cf8e7 into rollbear:main Oct 6, 2024
137 checks passed
@rollbear
Copy link
Owner

rollbear commented Oct 6, 2024

This is awesome. Thank you!

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