Skip to content

Commit

Permalink
Fix passing temporary const ref in indexable getter
Browse files Browse the repository at this point in the history
If both DefaultIndexableGetter has const ref pair argument, and
Indexables have decltype(auto) return, this ends up in a wrong result
(likely to the handling of temporaries). This leads to a
default-initialized scene bounding box, which leads to same Morton
codes, which leads to f-d up hierarchy.

Providing a second operator()(PairIndexVolume<Geometry> &&pair) operator
in DefaultIndexableGetter fixes that.
  • Loading branch information
aprokop committed Oct 25, 2023
1 parent 2338e1e commit 0fdfa87
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/details/ArborX_DetailsLegacy.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class LegacyValues
{}

KOKKOS_FUNCTION
decltype(auto) operator()(size_type i) const
auto operator()(size_type i) const
{
if constexpr (std::is_same_v<BoundingVolume,
typename AccessTraitsHelper<Access>::type>)
Expand Down
6 changes: 6 additions & 0 deletions src/details/ArborX_IndexableGetter.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ struct DefaultIndexableGetter
{
return pair.bounding_volume;
}

template <typename Geometry>
KOKKOS_FUNCTION Geometry operator()(PairIndexVolume<Geometry> &&pair) const
{
return pair.bounding_volume;
}
};

template <typename Values, typename IndexableGetter>
Expand Down
13 changes: 13 additions & 0 deletions test/tstDetailsTreeConstruction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,19 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(assign_morton_codes, DeviceType,
BOOST_TEST(ArborX::Details::equals(
scene_host, {{{0.0, 0.0, 0.0}}, {{(float)N, (float)N, (float)N}}}));

ArborX::Details::LegacyValues<decltype(boxes), ArborX::Box> values{boxes};
ArborX::Details::Indexables<decltype(values),
ArborX::Details::DefaultIndexableGetter>
indexables{values, ArborX::Details::DefaultIndexableGetter{}};

// Test for a bug where missing move ref operator() in DefaultIndexableGetter
// results in a default initialized indexable used in scene box calucation.
scene_host = ArborX::Box{};
ArborX::Details::TreeConstruction::calculateBoundingBoxOfTheScene(
space, indexables, scene_host);
BOOST_TEST(ArborX::Details::equals(
scene_host, {{{0.0, 0.0, 0.0}}, {{(float)N, (float)N, (float)N}}}));

Kokkos::View<unsigned long long *, DeviceType> morton_codes("morton_codes",
n);
ArborX::Details::TreeConstruction::projectOntoSpaceFillingCurve(
Expand Down

0 comments on commit 0fdfa87

Please sign in to comment.