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

Rework INFO lazy evaluation to use lambdas. #270

Merged
merged 8 commits into from Aug 12, 2019
Merged

Rework INFO lazy evaluation to use lambdas. #270

merged 8 commits into from Aug 12, 2019

Conversation

DaanDeMeyer
Copy link
Contributor

Instead of capturing everything by pointer, we capture the given
expression into a lazily evaluated lambda which we store as a reference.
This allows passing any expression to INFO including rvalues.

Fixes #269.

There's probably more code that can be removed but I wanted to get the initial version out there.

Instead of capturing everything by pointer, we capture the given
expression into a lazily evaluated lambda which we store as a reference.
This allows passing any expression to `INFO` including rvalues.
doctest/doctest.h Outdated Show resolved Hide resolved
doctest/parts/doctest_fwd.h Outdated Show resolved Hide resolved
doctest/doctest.h Outdated Show resolved Hide resolved
@DaanDeMeyer
Copy link
Contributor Author

I can't reproduce the Appveyor failure locally and the Appveyor debug logs aren't exactly helpful.

(Btw, you might want to apply for the Github actions beta (https://github.com/features/actions). If it turns out to be any good I'll likely move reproc's CI entirely to that and might do the same for doctest if it supports all necessary features).

@onqtam
Copy link
Member

onqtam commented Aug 11, 2019

I just noticed in the appveyor log that it is instantiating ContextScope with int o_O
https://ci.appveyor.com/project/onqtam/doctest/builds/26618380/job/edi528mpqshj2fj6#L132

maybe the decltype is the problem... we might experiment with passing the lambda by value instead of by reference... like here (without the perfect forwarding): https://stackoverflow.com/a/10270809/3162383

I could test it locally at home with MSVC.

I signed up for Actions, thanks.

This avoids conflicts with outer scope variables (which are all
captured by reference).
Use function template argument deduction via `MakeContextScope` instead
of `decltype` to declare instances of `ContextScope` in
`DOCTEST_CONFIG_IMPL`.
@DaanDeMeyer
Copy link
Contributor Author

Seems like MSVC just didn't like decltype on lambdas. I added a MakeContextScope instead to deduce the lambda type via template function argument deduction and that seems to work.

@onqtam
Copy link
Member

onqtam commented Aug 11, 2019

I guess clang will again complain about the extra semicolon in some places: {};

apart from that - this will probably work!

We don't remove the actual macro to keep backwards compatibility.
@DaanDeMeyer
Copy link
Contributor Author

DaanDeMeyer commented Aug 11, 2019

I made DOCTEST_TO_LVALUE a noop but didn't remove it to keep everything backwards compatible.

@DaanDeMeyer
Copy link
Contributor Author

That last commit only updates documentation so you don't necessarily have to wait for the entire build to complete again. The previous build completed successfully.

Mostly removal of constraints that can now be relaxed.
@onqtam onqtam merged commit 5e493d3 into doctest:dev Aug 12, 2019
@onqtam
Copy link
Member

onqtam commented Aug 12, 2019

good work - thanks for this! :)

onqtam pushed a commit that referenced this pull request Aug 12, 2019
…ld conflict with whatever is captured - relates #270 and #269
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.

None yet

2 participants