Skip to content

Commit

Permalink
Make conversion assigment explicit for Collection.
Browse files Browse the repository at this point in the history
This should prevent accidental construction-from-temporaries like the
one causing test failures in ValueGridBuilder.
  • Loading branch information
sethrj committed Mar 4, 2021
1 parent 4a9ae97 commit 6c1e091
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 25 deletions.
9 changes: 5 additions & 4 deletions scripts/docker/ci/launch-testing.sh
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
#!/bin/sh -ex
#!/bin/sh -e

if [ -z "$1" ]; then
echo "Usage: $0 pull-request-id" 1>&2
exit 2;
fi

CONTAINER=$(docker run -t -d celeritas/ci-cuda11:latest)
CONTAINER=$(docker run -t -d celeritas/ci-cuda11:2021-01-27)
echo "Launched container: ${CONTAINER}"
docker exec -i $CONTAINER bash -l <<EOF || echo "*BUILD FAILED*"
set -e
Expand All @@ -16,5 +16,6 @@ git checkout mr/$1
SOURCE_DIR=. BUILD_DIR=build entrypoint-shell ./scripts/build/docker.sh
EOF
docker stop --time=0 $CONTAINER
echo "To resume: docker exec -i -e 'TERM=xterm-256color' $CONTAINER bash -l"
echo "To delete: docker rm -f $CONTAINER"
echo "To resume: docker start $CONTAINER \\"
echo " && docker exec -it -e 'TERM=xterm-256color' $CONTAINER bash -l"
echo "To delete: docker rm -f $CONTAINER"
16 changes: 8 additions & 8 deletions src/base/Collection.hh
Original file line number Diff line number Diff line change
Expand Up @@ -121,25 +121,25 @@ class Collection

// Construct from another collection
template<Ownership W2, MemSpace M2>
inline Collection(const Collection<T, W2, M2, I>& other);
explicit inline Collection(const Collection<T, W2, M2, I>& other);

// Construct from another collection (mutable)
template<Ownership W2, MemSpace M2>
inline Collection(Collection<T, W2, M2, I>& other);
explicit inline Collection(Collection<T, W2, M2, I>& other);

//!@{
//! Default assignment
Collection& operator=(const Collection& other) = default;
Collection& operator=(Collection&& other) = default;
//!@}

// Assign from another collection in the same memory space
template<Ownership W2>
inline Collection& operator=(const Collection<T, W2, M, I>& other);
// Assign from another collectio
template<Ownership W2, MemSpace M2>
inline Collection& operator=(const Collection<T, W2, M2, I>& other);

// Assign (mutable!) from another collection in the same memory space
template<Ownership W2>
inline Collection& operator=(Collection<T, W2, M, I>& other);
// Assign (mutable!) from another collection
template<Ownership W2, MemSpace M2>
inline Collection& operator=(Collection<T, W2, M2, I>& other);

//// ACCESS ////

Expand Down
12 changes: 6 additions & 6 deletions src/base/Collection.i.hh
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@ Collection<T, W, M, I>::Collection(Collection<T, W2, M2, I>& other)

//---------------------------------------------------------------------------//
/*!
* Assign from another collection in the same memory space.
* Assign from another collection.
*/
template<class T, Ownership W, MemSpace M, class I>
template<Ownership W2>
template<Ownership W2, MemSpace M2>
Collection<T, W, M, I>&
Collection<T, W, M, I>::operator=(const Collection<T, W2, M, I>& other)
Collection<T, W, M, I>::operator=(const Collection<T, W2, M2, I>& other)
{
storage_ = detail::CollectionAssigner<W, M>()(other.storage_);
detail::CollectionStorageValidator<W2>()(this->size(),
Expand All @@ -52,12 +52,12 @@ Collection<T, W, M, I>::operator=(const Collection<T, W2, M, I>& other)

//---------------------------------------------------------------------------//
/*!
* Assign (mutable!) from another collection in the same memory space.
* Assign (mutable!) from another collection.
*/
template<class T, Ownership W, MemSpace M, class I>
template<Ownership W2>
template<Ownership W2, MemSpace M2>
Collection<T, W, M, I>&
Collection<T, W, M, I>::operator=(Collection<T, W2, M, I>& other)
Collection<T, W, M, I>::operator=(Collection<T, W2, M2, I>& other)
{
storage_ = detail::CollectionAssigner<W, M>()(other.storage_);
detail::CollectionStorageValidator<W2>()(this->size(),
Expand Down
4 changes: 3 additions & 1 deletion test/physics/em/LivermorePE.test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -505,8 +505,10 @@ TEST_F(LivermorePEInteractorTest, model)
EXPECT_EQ(1, grid_storage.size());

// Test cross sections calculated from tables
Collection<double, Ownership::const_reference, MemSpace::host> real_ref{
real_storage};
celeritas::XsCalculator calc_xs(
grid_storage[ValueGridInserter::XsIndex{0}], real_storage);
grid_storage[ValueGridInserter::XsIndex{0}], real_ref);
EXPECT_SOFT_EQ(0.1, calc_xs(MevEnergy{1e-3}));
EXPECT_SOFT_EQ(1e-5, calc_xs(MevEnergy{1e2}));
EXPECT_SOFT_EQ(1e-9, calc_xs(MevEnergy{1e6}));
Expand Down
9 changes: 7 additions & 2 deletions test/physics/grid/NonuniformGrid.test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,16 @@ class NonuniformGridTest : public celeritas::Test
{
auto build = celeritas::make_builder(&data);
irange = build.insert_back({0, 1, 3, 3, 7});
ref = data;
}

celeritas::ItemRange<int> irange;
celeritas::Collection<int, celeritas::Ownership::value, celeritas::MemSpace::host>
data;
celeritas::Collection<int,
celeritas::Ownership::const_reference,
celeritas::MemSpace::host>
ref;
};

//---------------------------------------------------------------------------//
Expand All @@ -39,7 +44,7 @@ class NonuniformGridTest : public celeritas::Test

TEST_F(NonuniformGridTest, accessors)
{
GridT grid(irange, data);
GridT grid(irange, ref);

EXPECT_EQ(5, grid.size());
EXPECT_EQ(0, grid.front());
Expand All @@ -50,7 +55,7 @@ TEST_F(NonuniformGridTest, accessors)

TEST_F(NonuniformGridTest, find)
{
GridT grid(irange, data);
GridT grid(irange, ref);

#if CELERITAS_DEBUG
EXPECT_THROW(grid.find(-1), celeritas::DebugError);
Expand Down
10 changes: 6 additions & 4 deletions test/physics/grid/ValueGridBuilder.test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,11 @@ class ValueGridBuilderTest : public celeritas::Test
{
b->build(insert);
}
real_ref = real_storage;
}

Collection<real_type, Ownership::value, MemSpace::host> real_storage;
Collection<real_type, Ownership::const_reference, MemSpace::host> real_ref;
Collection<XsGridData, Ownership::value, MemSpace::host> grid_storage;
};

Expand Down Expand Up @@ -75,13 +77,13 @@ TEST_F(ValueGridBuilderTest, xs_grid)
// Test results using the physics calculator
ASSERT_EQ(2, grid_storage.size());
{
XsCalculator calc_xs(grid_storage[XsIndex{0}], real_storage);
XsCalculator calc_xs(grid_storage[XsIndex{0}], real_ref);
EXPECT_SOFT_EQ(0.1, calc_xs(Energy{1e1}));
EXPECT_SOFT_EQ(0.2, calc_xs(Energy{1e2}));
EXPECT_SOFT_EQ(0.3, calc_xs(Energy{1e3}));
}
{
XsCalculator calc_xs(grid_storage[XsIndex{1}], real_storage);
XsCalculator calc_xs(grid_storage[XsIndex{1}], real_ref);
EXPECT_SOFT_EQ(10., calc_xs(Energy{1e-3}));
EXPECT_SOFT_EQ(1., calc_xs(Energy{1e-2}));
EXPECT_SOFT_EQ(0.1, calc_xs(Energy{1e-1}));
Expand All @@ -106,7 +108,7 @@ TEST_F(ValueGridBuilderTest, log_grid)
// Test results using the physics calculator
ASSERT_EQ(1, grid_storage.size());
{
XsCalculator calc_xs(grid_storage[XsIndex{0}], real_storage);
XsCalculator calc_xs(grid_storage[XsIndex{0}], real_ref);
EXPECT_SOFT_EQ(0.1, calc_xs(Energy{1e1}));
EXPECT_SOFT_EQ(0.2, calc_xs(Energy{1e2}));
EXPECT_SOFT_EQ(0.3, calc_xs(Energy{1e3}));
Expand All @@ -133,7 +135,7 @@ TEST_F(ValueGridBuilderTest, DISABLED_generic_grid)
// Test results using the physics calculator
ASSERT_EQ(2, grid_storage.size());
{
XsCalculator calc_xs(grid_storage[XsIndex{0}], real_storage);
XsCalculator calc_xs(grid_storage[XsIndex{0}], real_ref);
EXPECT_SOFT_EQ(0.1, calc_xs(Energy{1e1}));
EXPECT_SOFT_EQ(0.2, calc_xs(Energy{1e2}));
EXPECT_SOFT_EQ(0.3, calc_xs(Energy{1e3}));
Expand Down

0 comments on commit 6c1e091

Please sign in to comment.