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

Remove automatic collection of return values for iterations #784

Closed
bd82 opened this issue Jul 19, 2018 · 3 comments
Closed

Remove automatic collection of return values for iterations #784

bd82 opened this issue Jul 19, 2018 · 3 comments
Labels
Milestone

Comments

@bd82
Copy link
Member

bd82 commented Jul 19, 2018

The iterations DSL methods (MANY/AT_LEAST_ONE and _SEP variants)
automatically collect the return values of all iterations and return an array.

This causes some problems:

  • This still happens when a CST is produced but the results are never used.
  • In the use case of embedded methods we still may end up collecting information that is not needed.
    perhaps even collecting an array of "undefined" values.
  • This potentially unused capability comes as a performance cost:
    • See results of removing this behaivor from only MANY DSL method.
    screen shot 2018-07-19 at 6 04 50 pm

Need to investigate if anyone actually uses these APIs.
And if they will be removed also update relevant docs.

@bd82 bd82 added the API label Jul 19, 2018
@bd82 bd82 added this to the 4.0 milestone Jul 19, 2018
@bd82
Copy link
Member Author

bd82 commented Sep 17, 2018

While it does not make much sense to collect the iteration results when building a CST.
It does make sense to re-use those results when using embedded actions.
It would also be more consistent with the behavior of other DSL methods such as OR.

Perhaps it would be possible to optionally collect the iteration results only in embedded actions mode?

@bd82
Copy link
Member Author

bd82 commented Sep 19, 2018

After some more thinking I've decided to remove the automatic result collection completely because:

  1. CST creation (mostly) makes the the iteration results irrelevant and CST creation would become the default in version 4+.
  2. The automatic results collection can be useful when using embedded actions, but it is just a convenience, one can always manually collect the results.
  3. Lack of flexibility - when it does not fit one still pays the performance overhead.
  4. The performance overhead can reach 10% (Parser only) which is too much for a little convenience.

Conditionally enabling / disabling the results collection is likely possible but the added
complexity may not be worth it.

Keeping all the DSL methods as expressions (returns something) is also purer in a theoretical sense,
but the problem is being theoretically correct versus faster and simpler in the common case.

@bd82
Copy link
Member Author

bd82 commented Sep 19, 2018

Breaking changes will be tracked in
http://sap.github.io/chevrotain/docs/changes/BREAKING_CHANGES.html#_4-0-0

@bd82 bd82 closed this as completed in #818 Sep 19, 2018
@bd82 bd82 changed the title Evaluate removing automatic collection of return values for iterations REMOVE automatic collection of return values for iterations Sep 25, 2018
@bd82 bd82 changed the title REMOVE automatic collection of return values for iterations Remove automatic collection of return values for iterations Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant