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

Intervals.translate(Interval, long, int) and Intervals.translate(Interval, long... ) shadow each other #243

Open
maarzt opened this issue Feb 1, 2019 · 4 comments

Comments

@maarzt
Copy link
Contributor

maarzt commented Feb 1, 2019

The problem is that a call to the method Intervals.translate(Interval interval, long... translation) with two arguments is overloaded by the method Intervals.translate(Interval interval, long shift, int dim).

So a user that writes:

Intervals.translate(interval, 4, 5)

isn't sure which method is called, and what the result actually means. There's the idea to rename the method to Intervals.translateDim(Interval interval, long shift, int dim). At the same time Intervals.translate(Interval interval, long shift, int dim) should not be removed, but be deprecated, throw an Exception, have another return type maybe void. Such that code which uses this method notices the problem and is fixed. Otherwise a very subtle bug will be introduced.

There's a similar problem with Intervals.expand and Views.hyperSlice. Views.permuteCoordinates might also be worth looking at.

The problem is mentioned in #238

@chaubold
Copy link
Contributor

chaubold commented Feb 1, 2019

Thanks for setting up the issue! But I think you wanted to write Intervals.translate, not Views.translate, right?

@maarzt
Copy link
Contributor Author

maarzt commented Feb 1, 2019

No it's correct as it is. The problem is in the Intervals class, not in Views

@chaubold
Copy link
Contributor

chaubold commented Feb 1, 2019

The title is fine, but in the description you write

At the same time Views.translate(Interval interval, long shift, int dim) should not be removed

But there is no Views.translate(Interval interval, long shift, int dim), there is only Intervals.translate(Interval interval, long shift, int dim) -- as you noted yourself in #238 (comment) 🤔

@maarzt
Copy link
Contributor Author

maarzt commented Feb 4, 2019

The title is fine, but in the description you write

At the same time Views.translate(Interval interval, long shift, int dim) should not be removed

Thanks, for pointing that out

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

No branches or pull requests

2 participants