Skip to content

Conversation

blegat
Copy link
Member

@blegat blegat commented Nov 24, 2021

Follow up of #1682

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

I considered this, but I wasn't convinced it was the right approach.

Solvers using the get_fallback can call check_result_index_bounds in their method. It doesn't have to be in the get_fallback.

This is also inconsistent with the other get_fallback method which don't check that the result is in-bounds.

It also means for the caching optimizer method that we enter the try, potentially throw a result-index error, enter the catch, and then re-throw the error.

I decided it was simpler just to check before the try-catch.

@blegat
Copy link
Member Author

blegat commented Nov 24, 2021

Solvers using the get_fallback can call check_result_index_bounds in their method. It doesn't have to be in the get_fallback.

Yes but we might as well do it once in get_fallback rather than everywhere it's called from.

This is also inconsistent with the other get_fallback method which don't check that the result is in-bounds.

The other ones should be changed. They were written in a time we were not checking these result bounds yet.

It also means for the caching optimizer method that we enter the try, potentially throw a result-index error, enter the catch, and then re-throw the error.

That's worrying. It could also catch an InvalidIndex error. Shouldn't the catch check that the error is what it expects and rethrow otherwise ?

@odow odow closed this in #1702 Dec 14, 2021
@odow odow deleted the bl/check_result_fallback branch January 11, 2022 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants