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

adaptations for RangeDimension indexOf methods #819

Merged
merged 27 commits into from
Aug 19, 2020
Merged

Conversation

jgrewe
Copy link
Member

@jgrewe jgrewe commented May 7, 2020

This pull request is the first step for fixing the issue raised in #818
Adds some overloads to the RangeDimension::indexOf method family that allow for some behavior controlling. Unfortunately, the original indexOf behavior needed to be altered a bit. Invalid positions will not raise an exception. There is, however a new method that allows for cheking if a position is in range.

I made some decisions I would like to discuss:

  1. is it a good idea to allow end be less than start? At the moment I check and simply swap the variables (line 467 in Dimension.cpp).
  2. is it a good idea to silently ignore invalid ranges? Currently they are simply not part of the result vector. Alternatively the vector could contain optionals.
  3. The method for getting start and end indices of a single range (start and end positions) now also takes a vector of ticks (lines 460 et seq.). This to avoid mulitple retrievals of the ticks which would happen because this method might be repeatedly called by the vector-of-starts-and-ends version. Issue is, that I want to have the ticks argument optional. , i.e. method calls with {} instead of a vector. This forbids to pass the ticks vector as reference. So there is some copying going on, is there a good way to avoid this?

edit: test failures are expected :)

edit 2: I think this thing is good for review. Regarding the questions stressed above, the current states are:

  1. yes, I think so and this done exactly this way
  2. no, it is not. There are overloaded functions that return optionals which are not initialised for invalid ranges. Original functions (deprecated) will throw errors.
  3. kept it like that, tried in an intermediate version an rValue reference but, (according to @gicmo) repeated std::move is not a good idea, so this was removed again

@jgrewe jgrewe marked this pull request as ready for review May 23, 2020 10:24
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

I like it. It gets a little intense with the number of options, but I think the defaults are good so most users wont have to worry about it. The one default I would change is the inclusive/exclusive range matching. I would have preferred exclusive to be the default, though I get why it makes more sense to keep it inclusive since it was the previous default behaviour.

I like the default strict mode when searching for multiple indexes/ranges.

I don't think it's a good idea to silently swap start and end when end < start though (noted below).

src/Dimensions.cpp Show resolved Hide resolved
src/Dimensions.cpp Show resolved Hide resolved
jgrewe added 24 commits July 28, 2020 16:32
this overload uses the PositionMatch enum. Added tests for it.
adapt the method implementation to use new getIndex function
which now takes takes (optionally) the ticks, and the RangeMatch arg
to control inclusive or exclusive range matching
to use the optional variants of indexOf, invalid ranges are "silently"
ignored
…im ...

This is to define whether invalid start - end ranges are silently ignored
or in an exception should be thrown
to control whether invalid ranges lead to an exception,
positions less than offset now directly lead to an OutOfBounds error,
add an overload to simplify function calls, adapt the tests accordingly
overloads for optionals that support range and position matching,
respective tests
@achilleas-k achilleas-k merged commit f3bc13b into G-Node:1.4 Aug 19, 2020
@jgrewe jgrewe deleted the index_of branch November 29, 2020 10:29
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.

2 participants