From d3061b03ef36df41924bc12d7c1916326bbaaae5 Mon Sep 17 00:00:00 2001 From: Greg Landrum Date: Thu, 20 Jun 2024 09:27:27 +0200 Subject: [PATCH] Fixes github #7533 (#7539) * Fixes #7533 Fixes problem with SparseIntVect and SparseBitVect where the intention is to be able to store all possible values of the index type: in these cases you could not store (or access) the final element. * increase the verbosity of the DataStructs tests so that we can see what's going on --- Code/DataStructs/CMakeLists.txt | 3 +++ Code/DataStructs/SparseBitVect.cpp | 22 +++++++--------- Code/DataStructs/SparseBitVect.h | 11 +++++++- Code/DataStructs/SparseIntVect.h | 15 ++++++++--- Code/DataStructs/catch_tests.cpp | 29 ++++++++++++++++++++++ Code/DataStructs/testDatastructs.cpp | 4 +-- Code/GraphMol/Fingerprints/catch_tests.cpp | 8 ++++++ 7 files changed, 72 insertions(+), 20 deletions(-) create mode 100644 Code/DataStructs/catch_tests.cpp diff --git a/Code/DataStructs/CMakeLists.txt b/Code/DataStructs/CMakeLists.txt index 7c4ca5e0f..7e3f98a97 100644 --- a/Code/DataStructs/CMakeLists.txt +++ b/Code/DataStructs/CMakeLists.txt @@ -35,6 +35,9 @@ rdkit_test(testFPB testFPB.cpp rdkit_test(testMultiFPB testMultiFPB.cpp LINK_LIBRARIES DataStructs ) +rdkit_catch_test(catchDataStructs catch_tests.cpp + LINK_LIBRARIES DataStructs ) + if(RDK_BUILD_PYTHON_WRAPPERS) add_subdirectory(Wrap) endif() diff --git a/Code/DataStructs/SparseBitVect.cpp b/Code/DataStructs/SparseBitVect.cpp index 8afadfa5a..801bf8aae 100644 --- a/Code/DataStructs/SparseBitVect.cpp +++ b/Code/DataStructs/SparseBitVect.cpp @@ -1,6 +1,5 @@ -// $Id$ // -// Copyright (c) 2001-2008 greg Landrum and Rational Discovery LLC +// Copyright (c) 2001-2024 greg Landrum and other RDKit contributors // // @@ All Rights Reserved @@ // This file is part of the RDKit. @@ -14,7 +13,6 @@ #include "base64.h" #include #include -#include #ifdef WIN32 #include @@ -150,7 +148,7 @@ SparseBitVect SparseBitVect::operator~() const { // // """ ------------------------------------------------------- bool SparseBitVect::getBit(const unsigned int which) const { - if (which >= d_size) { + if (!checkIndex(which)) { throw IndexErrorException(which); } return dp_bits->count(which) > 0u; @@ -163,7 +161,7 @@ bool SparseBitVect::getBit(const unsigned int which) const { // // """ ------------------------------------------------------- bool SparseBitVect::getBit(const IntVectIter which) const { - if (*which < 0 || static_cast(*which) >= d_size) { + if (!checkIndex(which)) { throw IndexErrorException(*which); } return dp_bits->count(*which) > 0u; @@ -176,7 +174,7 @@ bool SparseBitVect::getBit(const IntVectIter which) const { // // """ ------------------------------------------------------- bool SparseBitVect::getBit(const IntSetIter which) const { - if (*which < 0 || static_cast(*which) >= d_size) { + if (!checkIndex(which)) { throw IndexErrorException(*which); } return dp_bits->count(*which) > 0u; @@ -193,11 +191,10 @@ bool SparseBitVect::setBit(const unsigned int which) { if (!dp_bits) { throw ValueErrorException("BitVect not properly initialized."); } - std::pair res; - if (which >= d_size) { + if (!checkIndex(which)) { throw IndexErrorException(which); } - res = dp_bits->insert(which); + auto res = dp_bits->insert(which); return !(res.second); } @@ -213,11 +210,10 @@ bool SparseBitVect::setBit(const IntSetIter which) { if (!dp_bits) { throw ValueErrorException("BitVect not properly initialized."); } - std::pair res; - if (*which < 0 || static_cast(*which) >= d_size) { + if (!checkIndex(which)) { throw IndexErrorException(*which); } - res = dp_bits->insert(*which); + auto res = dp_bits->insert(*which); return !(res.second); } @@ -232,7 +228,7 @@ bool SparseBitVect::unsetBit(const unsigned int which) { if (!dp_bits) { throw ValueErrorException("BitVect not properly initialized."); } - if (which >= d_size) { + if (!checkIndex(which)) { throw IndexErrorException(which); } diff --git a/Code/DataStructs/SparseBitVect.h b/Code/DataStructs/SparseBitVect.h index c70706671..879f8da47 100644 --- a/Code/DataStructs/SparseBitVect.h +++ b/Code/DataStructs/SparseBitVect.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2003-2008 greg Landrum and Rational Discovery LLC +// Copyright (C) 2007-2024 Greg Landrum and other RDKit contributors // // @@ All Rights Reserved @@ // This file is part of the RDKit. @@ -17,6 +17,7 @@ using std::set; #include #include +#include typedef set IntSet; typedef IntSet::iterator IntSetIter; @@ -96,6 +97,14 @@ class RDKIT_DATASTRUCTS_EXPORT SparseBitVect : public BitVect { private: unsigned int d_size{0}; void _initForSize(const unsigned int size) override; + bool checkIndex(const unsigned int idx) const { + return idx < d_size || (idx == d_size && + d_size == std::numeric_limits::max()); + } + template + bool checkIndex(const T which) const { + return *which >= 0 && static_cast(*which) < d_size; + } }; #endif diff --git a/Code/DataStructs/SparseIntVect.h b/Code/DataStructs/SparseIntVect.h index a75ebb7da..9c96aa138 100644 --- a/Code/DataStructs/SparseIntVect.h +++ b/Code/DataStructs/SparseIntVect.h @@ -1,6 +1,5 @@ -// $Id$ // -// Copyright (C) 2007-2008 Greg Landrum +// Copyright (C) 2007-2024 Greg Landrum and other RDKit contributors // // @@ All Rights Reserved @@ // This file is part of the RDKit. @@ -19,6 +18,7 @@ #include #include #include +#include const int ci_SPARSEINTVECT_VERSION = 0x0001; //!< version number to use in pickles @@ -75,7 +75,7 @@ class SparseIntVect { #endif //! return the value at an index int getVal(IndexType idx) const { - if (idx < 0 || idx >= d_length) { + if (!checkIndex(idx)) { throw IndexErrorException(static_cast(idx)); } int res = 0; @@ -88,7 +88,7 @@ class SparseIntVect { //! set the value at an index void setVal(IndexType idx, int val) { - if (idx < 0 || idx >= d_length) { + if (!checkIndex(idx)) { throw IndexErrorException(static_cast(idx)); } if (val != 0) { @@ -410,6 +410,13 @@ class SparseIntVect { d_data[tVal] = val; } } + bool checkIndex(IndexType idx) const { + if (idx < 0 || idx > d_length || + (idx == d_length && d_length < std::numeric_limits::max())) { + return false; + } + return true; + } }; template diff --git a/Code/DataStructs/catch_tests.cpp b/Code/DataStructs/catch_tests.cpp new file mode 100644 index 000000000..cbd6976cf --- /dev/null +++ b/Code/DataStructs/catch_tests.cpp @@ -0,0 +1,29 @@ +// +// Copyright (C) 2024 Greg Landrum and other RDKit contributors +// +// @@ All Rights Reserved @@ +// This file is part of the RDKit. +// The contents are covered by the terms of the BSD license +// which is included in the file license.txt, found at the root +// of the RDKit source tree. +// + +#include + +#include +#include "BitVects.h" +#include "BitOps.h" +#include "BitVectUtils.h" +#include "SparseIntVect.h" +#include + +using namespace RDKit; + +TEST_CASE("special cases for the limits of sparse vectors") { + SECTION("SparseBitVect") { + SparseBitVect sbv(std::numeric_limits::max()); + CHECK(sbv.getNumBits() == std::numeric_limits::max()); + CHECK(!sbv.setBit(std::numeric_limits::max())); + CHECK(sbv.getBit(std::numeric_limits::max()) == 1); + } +} \ No newline at end of file diff --git a/Code/DataStructs/testDatastructs.cpp b/Code/DataStructs/testDatastructs.cpp index a724b36e4..4a1b3d0e8 100644 --- a/Code/DataStructs/testDatastructs.cpp +++ b/Code/DataStructs/testDatastructs.cpp @@ -1,6 +1,5 @@ -// $Id$ // -// Copyright (C) 2001-2014 Greg Landrum and Rational Discovery LLC +// Copyright (C) 2001-2024 Greg Landrum and other RDKit contributors // // @@ All Rights Reserved @@ // This file is part of the RDKit. @@ -1453,6 +1452,7 @@ void test17Github3994() { int main() { RDLog::InitLogs(); + boost::logging::enable_logs("rdApp.info"); try { throw IndexErrorException(3); } catch (IndexErrorException &) { diff --git a/Code/GraphMol/Fingerprints/catch_tests.cpp b/Code/GraphMol/Fingerprints/catch_tests.cpp index 2ac7dbc31..cc7070543 100644 --- a/Code/GraphMol/Fingerprints/catch_tests.cpp +++ b/Code/GraphMol/Fingerprints/catch_tests.cpp @@ -794,3 +794,11 @@ TEST_CASE( } } } + +TEST_CASE("github #7533: IndexError while computing fingerprint") { + auto mol = "CC1C(B(C)C)S1(C)(C)=O"_smiles; + REQUIRE(mol); + auto fp = MorganFingerprints::getFingerprint(*mol, 2); + REQUIRE(fp); + CHECK(fp->getLength() == std::numeric_limits::max()); +} \ No newline at end of file