From b31a7c6eef286cbecebdafb8420c5b410d86925b Mon Sep 17 00:00:00 2001 From: Mark Abraham Date: Wed, 18 Mar 2015 08:52:15 +0000 Subject: [PATCH] Add unit tests for ArrayRef This fixes mdrun signalling on BG/Q. The templated constructor for ArrayRef does not work with xlc 12 on BG/Q with array fields of structs unless the base type has size equal to char. Presumably this is another example of the way that attributes of struct fields just get thrown away by compilers. Changed name of local variable to sig, just in case "signal" clashes with a preprocessor symbol somewhere... Some changes to the arrayref.h implementation to make it easier to write the tests as type-parameterized, by making the factory functions also appear as members of the corresponding classes, with the same name for both ArrayRef and ConstArrayRef. While there, remove some documentation duplication. Fixes #1701 Change-Id: I6894706b224dc5f3db7893503371107f1ff324d2 --- src/gromacs/mdlib/mdrun_signalling.cpp | 11 +- src/gromacs/mdlib/mdrun_signalling.h | 10 +- src/gromacs/utility/arrayref.h | 206 +++++++++++++----------- src/gromacs/utility/tests/CMakeLists.txt | 3 +- src/gromacs/utility/tests/arrayref.cpp | 266 +++++++++++++++++++++++++++++++ 5 files changed, 393 insertions(+), 103 deletions(-) create mode 100644 src/gromacs/utility/tests/arrayref.cpp diff --git a/src/gromacs/mdlib/mdrun_signalling.cpp b/src/gromacs/mdlib/mdrun_signalling.cpp index c76c4f7980..1714753f6b 100644 --- a/src/gromacs/mdlib/mdrun_signalling.cpp +++ b/src/gromacs/mdlib/mdrun_signalling.cpp @@ -88,10 +88,11 @@ prepareSignalBuffer(struct gmx_signalling_t *gs) { if (gs) { - gmx::ArrayRef signal(gs->sig); + gmx::ArrayRef sig(gs->sig); gmx::ArrayRef temp(gs->mpiBuffer); - std::copy(signal.begin(), signal.end(), temp.begin()); + std::copy(sig.begin(), sig.end(), temp.begin()); + return temp; } else @@ -130,9 +131,9 @@ handleSignals(struct gmx_signalling_t *gs, /* Set the communicated signal only when it is non-zero, * since signals might not be processed at each MD step. */ - int gsi = (gs->mpiBuffer[i] >= 0.0 ? - (int)(gs->mpiBuffer[i] + 0.5) : - (int)(gs->mpiBuffer[i] - 0.5)); + char gsi = (gs->mpiBuffer[i] >= 0.0 ? + static_cast(gs->mpiBuffer[i] + 0.5) : + static_cast(gs->mpiBuffer[i] - 0.5)); if (gsi != 0) { gs->set[i] = gsi; diff --git a/src/gromacs/mdlib/mdrun_signalling.h b/src/gromacs/mdlib/mdrun_signalling.h index 2fdde5d1bf..8f3c0340da 100644 --- a/src/gromacs/mdlib/mdrun_signalling.h +++ b/src/gromacs/mdlib/mdrun_signalling.h @@ -72,11 +72,15 @@ enum { eglsCHKPT, eglsSTOPCOND, eglsRESETCOUNTERS, eglsNR }; -/*! \internal \brief Object used by mdrun ranks to signal to each other */ +/*! \internal + * \brief Object used by mdrun ranks to signal to each other + * + * Note that xlc on BG/Q requires sig to be of size char (see unit tests + * of ArrayRef for details). */ struct gmx_signalling_t { int nstms; /**< The frequency for inter-simulation communication */ - int sig[eglsNR]; /**< The signal set by this rank in do_md */ - int set[eglsNR]; /**< The communicated signal, equal for all ranks once communication has occurred */ + char sig[eglsNR]; /**< The signal set by this rank in do_md */ + char set[eglsNR]; /**< The communicated signal, equal for all ranks once communication has occurred */ real mpiBuffer[eglsNR]; /**< Buffer for communication */ }; diff --git a/src/gromacs/utility/arrayref.h b/src/gromacs/utility/arrayref.h index d59a354857..ccf6e5e9d0 100644 --- a/src/gromacs/utility/arrayref.h +++ b/src/gromacs/utility/arrayref.h @@ -126,6 +126,51 @@ class ArrayRef typedef std::reverse_iterator const_reverse_iterator; /*! \brief + * Constructs a reference to a particular range from two pointers. + * + * \param[in] begin Pointer to the beginning of a range. + * \param[in] end Pointer to the end of a range. + * + * Passed pointers must remain valid for the lifetime of this object. + */ + static ArrayRef + fromPointers(value_type *begin, value_type *end) + { + return ArrayRef(begin, end); + } + /*! \brief + * Constructs a reference to an array. + * + * \param[in] begin Pointer to the beginning of the array. + * May be NULL if \p size is zero. + * \param[in] size Number of elements in the array. + * + * Passed pointer must remain valid for the lifetime of this object. + */ + static ArrayRef + fromArray(value_type *begin, size_t size) + { + return ArrayRef(begin, begin+size); + } + /*! \brief + * Constructs a reference to a particular range in a std::vector. + * + * \param[in] begin Iterator to the beginning of a range. + * \param[in] end Iterator to the end of a range. + * + * The referenced vector must remain valid and not be reallocated for + * the lifetime of this object. + */ + static ArrayRef + fromVector(typename std::vector::iterator begin, + typename std::vector::iterator end) + { + value_type *p_begin = (begin != end) ? &*begin : NULL; + value_type *p_end = p_begin + (end-begin); + return ArrayRef(p_begin, p_end); + } + + /*! \brief * Constructs an empty reference. */ ArrayRef() : begin_(NULL), end_(NULL) {} @@ -186,6 +231,11 @@ class ArrayRef * * This constructor is not explicit to allow directly passing * a C array to a function that takes an ArrayRef parameter. + * + * xlc on BG/Q compiles wrong code if the C array is a struct + * field, unless value_type is char or unsigned char. There's + * no good way to assert on this before C++11 (which that + * compiler will never support). */ template ArrayRef(value_type (&array)[count]) @@ -272,60 +322,6 @@ class ArrayRef }; -/*! \brief - * Constructs a reference to a particular range from two pointers. - * - * \param[in] begin Pointer to the beginning of a range. - * \param[in] end Pointer to the end of a range. - * - * Passed pointers must remain valid for the lifetime of this object. - * - * \related ArrayRef - */ -template -ArrayRef arrayRefFromPointers(T * begin, T * end) -{ - return ArrayRef(begin, end); -} - -/*! \brief - * Constructs a reference to an array - * - * \param[in] begin Pointer to the beginning of the array. - * May be NULL if \p size is zero. - * \param[in] size Number of elements in the array. - * - * Passed pointer must remain valid for the lifetime of this object. - * - * \related ArrayRef - */ -template -ArrayRef arrayRefFromArray(T * begin, size_t size) -{ - return arrayRefFromPointers(begin, begin+size); -} - -/*! \brief - * Constructs a reference to a particular range in a std::vector. - * - * \param[in] begin Iterator to the beginning of a range. - * \param[in] end Iterator to the end of a range. - * - * The referenced vector must remain valid and not be reallocated for - * the lifetime of this object. - * - * \related ArrayRef - */ -template -ArrayRef arrayRefFromVector(typename std::vector::iterator begin, - typename std::vector::iterator end) -{ - T * p_begin = (begin != end) ? &*begin : NULL; - T * p_end = p_begin + (end-begin); - return arrayRefFromPointers(p_begin, p_end); -} - - /*! \brief * STL-like container for non-mutable interface to a C array (or part of a @@ -381,6 +377,28 @@ class ConstArrayRef //! Standard reverse iterator. typedef std::reverse_iterator const_reverse_iterator; + //! \copydoc ArrayRef::fromPointers() + static ConstArrayRef + fromPointers(const value_type *begin, const value_type *end) + { + return ConstArrayRef(begin, end); + } + //! \copydoc ArrayRef::fromArray() + static ConstArrayRef + fromArray(const value_type *begin, size_t size) + { + return ConstArrayRef(begin, begin+size); + } + //! \copydoc ArrayRef::fromVector() + static ConstArrayRef + fromVector(typename std::vector::const_iterator begin, + typename std::vector::const_iterator end) + { + const value_type *p_begin = (begin != end) ? &*begin : NULL; + const value_type *p_end = p_begin + (end-begin); + return ConstArrayRef(p_begin, p_end); + } + /*! \brief * Constructs an empty reference. */ @@ -437,10 +455,16 @@ class ConstArrayRef * pointer). It constructs a reference to the whole array, without * a need to pass the number of elements explicitly. The compiler * must be able to deduce the array size. + * * Passed array must remain valid for the lifetime of this object. * * This constructor is not explicit to allow directly passing * a C array to a function that takes a ConstArrayRef parameter. + * + * xlc on BG/Q compiles wrong code if the C array is a struct + * field, unless value_type is char or unsigned char. There's + * no good way to assert on this before C++11 (which that + * compiler will never support). */ template ConstArrayRef(const value_type (&array)[count]) @@ -502,57 +526,51 @@ class ConstArrayRef }; -/*! \brief - * Constructs a reference to a particular range from two pointers. - * - * \param[in] begin Pointer to the beginning of a range. - * \param[in] end Pointer to the end of a range. - * - * Passed pointers must remain valid for the lifetime of this object. - * - * \related ConstArrayRef - */ +//! \copydoc ArrayRef::fromPointers() +//! \related ArrayRef template -ConstArrayRef constArrayRefFromPointers(const T * begin, const T * end) +ArrayRef arrayRefFromPointers(T *begin, T *end) { - return ConstArrayRef(begin, end); + return ArrayRef::fromPointers(begin, end); } - -/*! \brief - * Constructs a reference to an array. - * - * \param[in] begin Pointer to the beginning of the array. - * May be NULL if \p size is zero. - * \param[in] size Number of elements in the array. - * - * Passed pointer must remain valid for the lifetime of this object. - * - * \related ConstArrayRef - */ +//! \copydoc ArrayRef::fromArray() +//! \related ArrayRef template -ConstArrayRef constArrayRefFromArray(const T * begin, size_t size) +ArrayRef arrayRefFromArray(T *begin, size_t size) { - return constArrayRefFromPointers(begin, begin+size); + return ArrayRef::fromArray(begin, size); +} +//! \copydoc ArrayRef::fromVector() +//! \related ArrayRef +template +ArrayRef arrayRefFromVector(typename std::vector::iterator begin, + typename std::vector::iterator end) +{ + return ArrayRef::fromVector(begin, end); } -/*! \brief - * Constructs a reference to a particular range in a std::vector. - * - * \param[in] begin Iterator to the beginning of a range. - * \param[in] end Iterator to the end of a range. - * - * The referenced vector must remain valid and not be reallocated for - * the lifetime of this object. - * - * \related ConstArrayRef - */ + +//! \copydoc ConstArrayRef::fromPointers() +//! \related ConstArrayRef +template +ConstArrayRef constArrayRefFromPointers(const T *begin, const T *end) +{ + return ConstArrayRef::fromPointers(begin, end); +} +//! \copydoc ConstArrayRef::fromArray() +//! \related ConstArrayRef +template +ConstArrayRef constArrayRefFromArray(const T *begin, size_t size) +{ + return ConstArrayRef::fromArray(begin, size); +} +//! \copydoc ConstArrayRef::fromVector() +//! \related ConstArrayRef template ConstArrayRef constArrayRefFromVector(typename std::vector::const_iterator begin, typename std::vector::const_iterator end) { - const T * p_begin = (begin != end) ? &*begin : NULL; - const T * p_end = p_begin + (end-begin); - return constArrayRefFromPointers(p_begin, p_end); + return ConstArrayRef::fromVector(begin, end); } /*! \brief diff --git a/src/gromacs/utility/tests/CMakeLists.txt b/src/gromacs/utility/tests/CMakeLists.txt index 9024e56ea6..a002c762c3 100644 --- a/src/gromacs/utility/tests/CMakeLists.txt +++ b/src/gromacs/utility/tests/CMakeLists.txt @@ -1,7 +1,7 @@ # # This file is part of the GROMACS molecular simulation package. # -# Copyright (c) 2012,2013,2014, by the GROMACS development team, led by +# Copyright (c) 2012,2013,2014,2015, by the GROMACS development team, led by # Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, # and including many others, as listed in the AUTHORS file in the # top-level source directory and at http://www.gromacs.org. @@ -33,5 +33,6 @@ # the research papers on the package. Check out http://www.gromacs.org. gmx_add_unit_test(UtilityUnitTests utility-test + arrayref.cpp bitmask32.cpp bitmask64.cpp bitmask128.cpp stringutil.cpp) diff --git a/src/gromacs/utility/tests/arrayref.cpp b/src/gromacs/utility/tests/arrayref.cpp new file mode 100644 index 0000000000..d5f00bc056 --- /dev/null +++ b/src/gromacs/utility/tests/arrayref.cpp @@ -0,0 +1,266 @@ +/* + * This file is part of the GROMACS molecular simulation package. + * + * Copyright (c) 2015, by the GROMACS development team, led by + * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, + * and including many others, as listed in the AUTHORS file in the + * top-level source directory and at http://www.gromacs.org. + * + * GROMACS is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public License + * as published by the Free Software Foundation; either version 2.1 + * of the License, or (at your option) any later version. + * + * GROMACS is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with GROMACS; if not, see + * http://www.gnu.org/licenses, or write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + * If you want to redistribute modifications to GROMACS, please + * consider that scientific software is very special. Version + * control is crucial - bugs must be traceable. We will be happy to + * consider code for inclusion in the official distribution, but + * derived work must not be called official GROMACS. Details are found + * in the README & COPYING files - if they are missing, get the + * official version at http://www.gromacs.org. + * + * To help us fund GROMACS development, we humbly ask that you cite + * the research papers on the package. Check out http://www.gromacs.org. + */ +/*! \internal \file + * \brief Tests for gmx::ArrayRef and gmx::ConstArrayRef. + * + * \author Mark Abraham + * \ingroup module_utility + */ +#include "gmxpre.h" + +#include "gromacs/utility/arrayref.h" + +#include "config.h" + +#include + +#include + +#include "gromacs/utility/basedefinitions.h" +#include "gromacs/utility/real.h" + +namespace gmx +{ + +namespace +{ + +TEST(EmptyArrayRefTest, IsEmpty) +{ + EmptyArrayRef emptyArray = {}; + ArrayRef empty(emptyArray); + + EXPECT_EQ(0U, empty.size()); + EXPECT_TRUE(empty.empty()); +} + +TEST(EmptyConstArrayRefTest, IsEmpty) +{ + EmptyArrayRef emptyArray = {}; + ConstArrayRef empty(emptyArray); + + EXPECT_EQ(0U, empty.size()); + EXPECT_TRUE(empty.empty()); +} + +#ifdef GTEST_HAS_TYPED_TEST + +//! Define the types that end up being available as TypeParam in the test cases for both kinds of ArrayRef +typedef ::testing::Types< + ArrayRef, + ArrayRef, + ArrayRef, + ArrayRef, + ArrayRef, + ArrayRef, + ArrayRef, + ArrayRef, + ArrayRef, + ArrayRef, + ConstArrayRef, + ConstArrayRef, + ConstArrayRef, + ConstArrayRef, + ConstArrayRef, + ConstArrayRef, + ConstArrayRef, + ConstArrayRef, + ConstArrayRef, + ConstArrayRef + > ArrayRefTypes; + +/*! \brief Permit all the tests to run on all kinds of ArrayRefs + * + * The main objective is to verify that all the different kinds of + * construction lead to the expected result. */ +template +class ArrayRefTest : public ::testing::Test +{ + public: + typedef TypeParam ArrayRefType; + typedef typename ArrayRefType::value_type ValueType; + + /*! \brief Run the same tests all the time + * + * Note that test cases must call this->runTests(), because + * that's how the derived-class templates that implement + * type-parameterized tests actually work. */ + void runTests(ValueType *a, + size_t aSize, + ValueType *aData, + ArrayRefType &arrayRef) + { + ASSERT_EQ(aSize, arrayRef.size()); + ASSERT_FALSE(arrayRef.empty()); + EXPECT_EQ(aData, arrayRef.data()); + EXPECT_EQ(a[0], arrayRef.front()); + EXPECT_EQ(a[aSize-1], arrayRef.back()); + for (size_t i = 0; i != aSize; ++i) + { + EXPECT_EQ(a[i], arrayRef[i]); + } + } +}; + +TYPED_TEST_CASE(ArrayRefTest, ArrayRefTypes); + +/* Welcome back to the past. While you can declare a static array[] of + templated type in a class, in C++98, you have to define it outside + the class, and when you do, the compiler knows the declaration is + incomplete and can't match the types to actual functions. So, + declaring locals is the only choice available, so we need macros to + avoid duplication. Lovely. */ +#define DEFINE_ARRAY(a, aSize) \ + typename TestFixture::ValueType (a)[] = { \ + static_cast(1.2), \ + static_cast(2.4), \ + static_cast(3.1) \ + }; \ + size_t (aSize) = sizeof((a)) / sizeof(typename TestFixture::ValueType); + +TYPED_TEST(ArrayRefTest, MakeWithAssignmentWorks) +{ + DEFINE_ARRAY(a, aSize); + typename TestFixture::ArrayRefType arrayRef = a; + this->runTests(a, aSize, a, arrayRef); +} + +TYPED_TEST(ArrayRefTest, ConstructWithTemplateConstructorWorks) +{ + DEFINE_ARRAY(a, aSize); + typename TestFixture::ArrayRefType arrayRef(a); + this->runTests(a, aSize, a, arrayRef); +} + +TYPED_TEST(ArrayRefTest, ConstructFromPointersWorks) +{ + DEFINE_ARRAY(a, aSize); + typename TestFixture::ArrayRefType arrayRef(a, a + aSize); + this->runTests(a, aSize, a, arrayRef); +} + +TYPED_TEST(ArrayRefTest, MakeFromPointersWorks) +{ + DEFINE_ARRAY(a, aSize); + typename TestFixture::ArrayRefType arrayRef + = TestFixture::ArrayRefType::fromPointers(a, a + aSize); + this->runTests(a, aSize, a, arrayRef); +} + +TYPED_TEST(ArrayRefTest, MakeFromArrayWorks) +{ + DEFINE_ARRAY(a, aSize); + typename TestFixture::ArrayRefType arrayRef + = TestFixture::ArrayRefType::fromArray(a, aSize); + this->runTests(a, aSize, a, arrayRef); +} + +TYPED_TEST(ArrayRefTest, ConstructFromVectorWorks) +{ + DEFINE_ARRAY(a, aSize); + std::vector v(a, a + aSize); + typename TestFixture::ArrayRefType arrayRef(v); + this->runTests(a, aSize, v.data(), arrayRef); +} + +TYPED_TEST(ArrayRefTest, MakeFromVectorWorks) +{ + DEFINE_ARRAY(a, aSize); + std::vector v(a, a + aSize); + typename TestFixture::ArrayRefType arrayRef + = TestFixture::ArrayRefType::fromVector(v.begin(), v.end()); + this->runTests(a, aSize, v.data(), arrayRef); +} + +//! Helper struct for the case actually used in mdrun signalling +template +struct Helper +{ + public: + T a[3]; + int size; +}; + +/*! \brief Test of the case actually used in mdrun signalling + * + * There, we take a non-const struct-field array of static length and + * make an ArrayRef to it using the template constructor that is + * supposed to infer the length from the static size. But on xlc on + * BlueGene/Q, if the base type is not char (or unsigned char), the + * generated code ends up with an ArrayRef of zero size, so everything + * breaks. Presumably the default code path accidentally works for + * char. + * + * Fortunately, all current uses of that constructor have a base type + * of char, so there's no big problem. Using a pointer-based + * constructor does work, if there's ever a problem (and that is + * tested above). */ + +TYPED_TEST(ArrayRefTest, ConstructFromStructFieldWithTemplateConstructorWorks) +{ + DEFINE_ARRAY(a, aSize); + Helper h; + h.size = aSize; + for (int i = 0; i != h.size; ++i) + { + h.a[i] = a[i]; + } + typename TestFixture::ArrayRefType arrayRef(h.a); +#if defined(GMX_TARGET_BGQ) && defined(__xlC__) + if (sizeof(typename TestFixture::ValueType) == sizeof(char)) +#endif + { + this->runTests(h.a, h.size, h.a, arrayRef); + } +} + +#else // GTEST_HAS_TYPED_TEST + +/* A dummy test that at least signals that something is missing if one runs the + * unit test executable itself. + */ +TEST(DISABLED_ArrayRefTest, GenericTests) +{ + ADD_FAILURE() + << "Tests for generic ArrayRef functionality require support for " + << "Google Test typed tests, which was not available when the tests " + << "were compiled."; +} + +#endif // GTEST_HAS_TYPED_TEST + +} // namespace + +} // namespace -- 2.11.4.GIT