-
Notifications
You must be signed in to change notification settings - Fork 160
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
simplify with directive #239
Conversation
wanghan02
commented
Aug 21, 2017
•
edited
Loading
edited
- when used as x3::with(x), where x is an lvalue, we inject an lvalue reference of x to the context.
- when used as x3::with(x), where x is a const lvalue, we inject a const lvalue reference of x to the context.
- when used as x3::with(x), where x is an rvalue, we move the rvalue to the member of with_directive and inject an lvalue reference of that member to the context.
- No copy is applied; std::ref can still be used but not needed.
- remove unused with_context
1. when used as x3::with<ID>(x), where x is an lvalue, we inject an lvalue reference of x to the context. 2. when used as x3::with<ID>(x), where x is a const lvalue, we inject a const lvalue reference of x to the context. 3. when used as x3::with<ID>(x), where x is an rvalue, we move the rvalue to the member of with_directive and inject an lvalue reference of that member to the context. 4. No copy is applied; std::ref is not needed 5. remove unused with_context
Looks good! Can you provide some test files? |
@djowel Test cases have been added to test/x3/with.cpp
So I brought back the with_value_holder and change the T const specialization to T& specialization, and makes rvalue injection modifiable. Test cases have been changed to adapt to the new requirement. |
wonderful! |
…e `with_context` usage (ref boostorg/spirit#239)
: unary_parser<Subject, Derived> | ||
{ | ||
typedef unary_parser<Subject, Derived> base_type; | ||
mutable T val; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why is it mutable
?
- The tests you had added do not cover it.
- Allowing to mutate the value creates thread-safety risks.
- It does not play well with
constexpr
, especially with Clang 6 and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was rvalue injection should be mutable in const parser as well. Given parsers are immutable in the parse functions, rvalue injection would be immutable without mutable
keyword.
But I also agree with you on the issues it brings. I don't have a solution at the moment.
…e `with_context` usage (ref boostorg/spirit#239)