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

simplify with directive #239

Merged
merged 3 commits into from
Aug 22, 2017
Merged

simplify with directive #239

merged 3 commits into from
Aug 22, 2017

Conversation

wanghan02
Copy link
Contributor

@wanghan02 wanghan02 commented Aug 21, 2017

  1. when used as x3::with(x), where x is an lvalue, we inject an lvalue reference of x to the context.
  2. when used as x3::with(x), where x is a const lvalue, we inject a const lvalue reference of x to the context.
  3. 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.
  4. No copy is applied; std::ref can still be used but not needed.
  5. 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
@djowel
Copy link
Member

djowel commented Aug 21, 2017

Looks good! Can you provide some test files?

@wanghan02
Copy link
Contributor Author

wanghan02 commented Aug 22, 2017

@djowel Test cases have been added to test/x3/with.cpp
rvalue injection: The 1st commit actually inject a const ref to the member variable because parse(...) is a const method. This behaves the same as before (but without copying), but does not really make sense. I think the best way to use with directive is:

  • Inject non-const lvalue: able to modify during parsing, the original lvalue is modified.
  • Inject rvalue: able to modify during parsing.
  • Inject const lvalue: not able to modify during parsing.

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.

@djowel
Copy link
Member

djowel commented Aug 22, 2017

wonderful!

: unary_parser<Subject, Derived>
{
typedef unary_parser<Subject, Derived> base_type;
mutable T val;
Copy link
Collaborator

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?

  1. The tests you had added do not cover it.
  2. Allowing to mutate the value creates thread-safety risks.
  3. It does not play well with constexpr, especially with Clang 6 and below.

Copy link
Contributor Author

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.

leedejun pushed a commit to leedejun/mapnik that referenced this pull request Dec 26, 2023
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

3 participants