Add bounds checking to extractSubroute.

[malban]

Add bounds checking to extractSubroute to handle start and end positions being out of order.

[kriskozak]

Did you ask me to review this just to see if I was paying attention?

I'm not sure I follow all of the logic here 100%, but for this part, does this actually constitute a success? Or did you mean to return false. If it's fine, should you have cleared sub_route.points sub_route.points.clear() before returning here or up at the top of this function?

[malban]

I think it should return true. We asked for an empty subroute and we return an empty subtoute. Success!

The use case I'm thinking of is that a user requests a subroute that is [route start, route_end - 50m). If the route is less than 50m then this would be an empty route, which would not be a failure and would be the intuitive result.

[elliotjo]

I'd say either clear it right after the test and return true, or return false and don't touch sub_route. The first one seems like the most convenient to me (sort of in line with python slices).

…

On Thu, Sep 21, 2017 at 12:01 AM, kriskozak ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In swri_route_util/src/util.cpp <#486 (comment)> : > @@ -709,6 +709,11 @@ bool extractSubroute( end_index++; end_index = std::min(end_index, route.points.size()); + if (end_index <= start_index) + { + return true; + } + Did you ask me to review this just to see if I was paying attention? I'm not sure I follow all of the logic here 100%, but or this part, does this actually constitute a success? Or did you mean to return false. If it's fine, should you have cleared sub_route.points sub_route.points.clear() before returning here or up at the top of this function? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#486 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0H6Mj9z9C6JpuyiLwwYQUDEaGNIW2-ks5ske2egaJpZM4Pedku> .

[malban]

@elliotjo I think you're right the route points should be cleared in case the output variable wasn't clean to begin with. Does the rebuildPointIndex function need to get called then?

[elliotjo]

Yes, good call on rebuilding the point index.

…

On Sep 21, 2017 10:40 PM, "Marc Alban" ***@***.***> wrote: @elliotjo <https://github.com/elliotjo> I think you're right the route points should be cleared in case the output variable wasn't clean to begin with. Does the rebuildPointIndex function need to get called then? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#486 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0H6BAC8OUpv11_GPTKXkQbWImLCYQ2ks5skmBSgaJpZM4Pedku> .