-
Notifications
You must be signed in to change notification settings - Fork 88
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
Fix broken bounds checks #388
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, GKO_ASSERT_EQ
is not tested like it should be in core/test/base/exception_helpers.cpp
. Please add tests there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@upsj I forgot to point you towards the How to merge entry in our wiki. |
Some tests are failing due to changing to |
There were two types of failures: Mismatching exception types as @tcojean mentioned and some bounds checks indeed need to use |
94c3c7a
to
d508489
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -240,9 +240,7 @@ class SparsityCsr | |||
auto tmp = Array<value_type>{exec->get_master(), 1}; | |||
tmp.get_data()[0] = value; | |||
value_ = Array<value_type>{exec, std::move(tmp)}; | |||
GKO_ENSURE_IN_BOUNDS(col_idxs_.get_num_elems() - 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, 🤷♂️
This version of Ginkgo provides a few fixes in Ginkgo's core routines. The supported systems and requirements are unchanged from version 1.1.0. ### Fixes + Improve Ginkgo's installation and fix the `test_install` step ([#406](#406)), + Fix some documentation issues ([#406](#406)), + Fix multiple code issues reported by sonarqube ([#406](#406)), + Update the git-cmake-format repository ([#399](#399)), + Improve the global update header script ([#390](#390)), + Fix broken bounds checks ([#388](#388)), + Fix CSR strategies and improve performance ([#379](#379)), + Fix a small typo in the stencil examples ([#381](#381)), + Fix ELL error on small matrices ([#375](#375)), + Fix SellP read function ([#374](#374)), + Add factorization support in `create_new_algorithm.sh` ([#371](#371)).
Minor release v1.1.1 This version of Ginkgo provides a few fixes in Ginkgo's core routines. The supported systems and requirements are unchanged from version 1.1.0. ### Fixes + Fix the `test_install` step with `HIP` ([#409](#409)), + Improve Ginkgo's installation and fix the `test_install` step ([#406](#406)), + Fix some documentation issues ([#406](#406)), + Fix multiple code issues reported by sonarqube ([#406](#406)), + Update the git-cmake-format repository ([#399](#399)), + Improve the global update header script ([#390](#390)), + Fix broken bounds checks ([#388](#388)), + Fix CSR strategies and improve performance ([#379](#379)), + Fix a small typo in the stencil examples ([#381](#381)), + Fix ELL error on small matrices ([#375](#375)), + Fix SellP read function ([#374](#374)), + Add factorization support in `create_new_algorithm.sh` ([#371](#371))
As #387 explains, we have some bounds checks that should rather be checks for equality, unless we want to allow our
Array
s to over-allocate memory, e.g. for SIMD purposes or similar.This pull request replaces all
GKO_ENSURE_IN_BOUNDS
by equivalent or more strictGKO_ASSERT_EQ