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

added range_redesign rfc #147

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

added range_redesign rfc #147

wants to merge 8 commits into from

Conversation

chobeat
Copy link

@chobeat chobeat commented Apr 1, 2019

RFC for the range redesign. All the details are in text/0000_range_redesign.md

@SeanTAllen
Copy link
Member

Markdown formatting seems a little off based on the highlighting that GitHub is doing

@SeanTAllen
Copy link
Member

Looking at your reference code, its more explicit about inclusive and exclusive but reverse is now implicit. The reverse API is non-obvious in that fashion to me. I think this should be addressed in the RFC.

@jemc
Copy link
Member

jemc commented Apr 2, 2019

Can you include the important points of the API design in the RFC, along with a few usage examples?

I know you linked a reference implementation, but it's good to have the API design in the RFC itself for posterity, since the reference implementation link may become invalid later. Also, seeing usage examples would make this easier to talk about.

@chobeat
Copy link
Author

chobeat commented Apr 14, 2019

I've addressed all the points you raised and I think the document is ready for another round of review.


* be able to generate all the possible ranges, increasing and decreasing, for all the data types, without incurring in overflow errors.
* offer a unified, usable, coherent API with the possibility to specify inclusive or exclusive bounds. To achieve this, the definition of the bounds and the range should be independent. The Range class should offer helper methods to cover the most common cases without explicitely instancing the bounds.
* never raise errors but instead return an empty list. This decision arises from the assumption that a majority of ranges are defined with bounds known at compile time while declaring ranges using runtime values is less common. The error handling in that case has to be done by calling the `is_invalid()` method. A version of the class with explicit errors can be considered.
Copy link
Member

@SeanTAllen SeanTAllen Apr 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"majority of ranges are defined with bounds know at compile time" isn't my experience. if the majority of ranges were defined with bounds at runtime, would that change the "never raise errors" idea?

Copy link
Author

@chobeat chobeat Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, because the assumption is that ranges defined with hardcoded value don't need error handling. The range will be either always valid or always invalid. If this is not the most common scenario, raising an error would be more idiomatic than checking is_invalid. The idea is to renounce an idiomatic interface to save the user a lot of unnecessary error handling.

@SeanTAllen SeanTAllen added the status - final comment period The RFC is finalized. Waiting for final comments. label May 2, 2019
@jemc
Copy link
Member

jemc commented May 14, 2019

It was mentioned on today's sync call that this implementation adds some allocation overhead that will significantly slow down use of Range in, say, a for loop.

Having Inclusive and Exclusive be a class containing a number means that they will be allocated as objects. Having them behind the trait Bound means that we will use virtual calls and not reified, which precludes inlining of the function and stack-allocation of the class.

To fix the performance issue, Inclusive and Exclusive could each be a primitive with no state, and the number is kept in a separate field. Making it use a primitive and a number separately means no allocation for that data. Additionally, they should be type parameters instead of runtime parameters, and iftype used to check the type parameter, so that the inclusivity/exclusivity control flow paths can be known at compile-time instead of runtime.

From a usability standpoint, it is suggested that we make sure the defaults for those type parameters are matching the current behaviour of Range for compatibility purposes.

@sylvanc
Copy link
Contributor

sylvanc commented May 15, 2019

Another issue for me is that dynamically determining forward or backward would interfere with high performance code patterns. For example, writing a loop that was intentionally a forward loop but that might (dynamically) intentionally perform zero iterations (by setting the begin as higher than the end). This is common in, for example, matrix multiplication (a scenario of particular interest for me).

Much like @jemc says above for inclusive and exclusive, forward and backward could be encoded as primitive type parameters, allowing zero-cost reification.

@jemc
Copy link
Member

jemc commented Apr 28, 2020

Removing from final comment period until the above comments are addressed.

@jemc jemc removed the status - final comment period The RFC is finalized. Waiting for final comments. label Apr 28, 2020
Base automatically changed from master to main February 8, 2021 22:15
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.

5 participants