Skip to content

Commit

Permalink
Reimplement GenericMap using PolymorphicValue.
Browse files Browse the repository at this point in the history
  • Loading branch information
@kfindeisen
kfindeisen committed May 8, 2019
1 parent 1351795 commit e9f231f
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 107 deletions.
67 changes: 12 additions & 55 deletions include/lsst/afw/typehandling/GenericMap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@

#include "lsst/pex/exceptions.h"
#include "lsst/afw/typehandling/Storable.h"
#include "lsst/afw/typehandling/PolymorphicValue.h"

namespace lsst {
namespace afw {
Expand Down Expand Up @@ -280,9 +281,8 @@ class GenericMap {
try {
auto foo = unsafeLookup(key.getId());
// Don't use pointer-based get, because it won't work after migrating to std::variant
// return unique_ptr by reference, so the pointer in internal storage won't get cleared
auto& holder = boost::get<std::unique_ptr<Storable> const&>(foo);
T* typedPointer = dynamic_cast<T*>(holder.get());
Storable const& value = boost::get<PolymorphicValue const&>(foo);
T const* typedPointer = dynamic_cast<T const*>(&value);
if (typedPointer != nullptr) {
return *typedPointer;
} else {
Expand Down Expand Up @@ -420,9 +420,9 @@ class GenericMap {
auto foo = unsafeLookup(key.getId());
try {
// Don't use pointer-based get, because it won't work after migrating to std::variant
// return unique_ptr by reference, so the pointer in internal storage won't get cleared
auto& holder = boost::get<std::unique_ptr<Storable> const&>(foo);
return dynamic_cast<T*>(holder.get()) != nullptr;
Storable const& value = boost::get<PolymorphicValue const&>(foo);
auto asT = dynamic_cast<T const*>(&value);
return asT != nullptr;
} catch (boost::bad_get const&) {
return false;
}
Expand Down Expand Up @@ -483,9 +483,7 @@ class GenericMap {
return false;
}
for (K const& key : keys1) {
// objects conceptually held by value are copied and held by unique_ptr
// make sure to compare the objects being pointed instead
if (!_proxyEquals(this->unsafeLookup(key), other.unsafeLookup(key))) {
if (this->unsafeLookup(key) != other.unsafeLookup(key)) {
return false;
}
}
Expand All @@ -508,12 +506,12 @@ class GenericMap {
/**
* A type-agnostic reference to the value stored inside the map.
*
* Keys of any subclass of Storable are implemented using `unique_ptr<Storable>` to preserve type.
* Keys of any subclass of Storable are implemented using PolymorphicValue to preserve type.
*/
// may need to use std::reference_wrapper when migrating to std::variant, but it confuses Boost
using ValueReference = boost::variant<bool const&, std::int32_t const&, std::int64_t const&, float const&,
double const&, std::string const&, std::unique_ptr<Storable> const&,
std::shared_ptr<Storable> const&>;
using ValueReference =
boost::variant<bool const&, std::int32_t const&, std::int64_t const&, float const&, double const&,
std::string const&, PolymorphicValue const&, std::shared_ptr<Storable> const&>;

/**
* The types that can be stored in a map.
Expand Down Expand Up @@ -548,32 +546,6 @@ class GenericMap {
auto rawKeys = keys();
return std::unordered_set<K>(rawKeys.begin(), rawKeys.end());
}

class _ProxyComparator {
public:
template <typename T, typename U, typename std::enable_if_t<!std::is_same<T, U>::value, int> = 0>
bool operator()(T const& lhs, U const& rhs) const {
return false;
}
template <typename T>
bool operator()(T const& lhs, T const& rhs) const {
return lhs == rhs;
}
bool operator()(Storable const& lhs, Storable const& rhs) const { return lhs.equals(rhs); }
bool operator()(std::unique_ptr<Storable> const& lhs, std::unique_ptr<Storable> const& rhs) const {
return (*lhs).equals(*rhs);
}
};

/**
* Equality test for internal representations of objects.
*
* Storable behaves as if held by value, but actually held by unique_ptr.
* Override unique_ptr comparisons to compare the objects, not the pointers.
*/
bool _proxyEquals(ValueReference const& value1, ValueReference const& value2) const noexcept {
return boost::apply_visitor(_ProxyComparator(), value1, value2);
}
};

/**
Expand Down Expand Up @@ -618,11 +590,8 @@ class MutableGenericMap : public GenericMap<K> {
*
* @note This implementation calls @ref contains(K const&) const "contains",
* then calls @ref unsafeInsert if there is no conflicting key.
*
* @{
*/
// Can't partially specialize method templates, rely on enable_if to avoid duplicates
template <typename T, typename std::enable_if_t<!std::is_base_of<Storable, T>::value, int> = 0>
template <typename T>
bool insert(Key<K, T> const& key, T const& value) {
if (this->contains(key.getId())) {
return false;
Expand All @@ -631,18 +600,6 @@ class MutableGenericMap : public GenericMap<K> {
return unsafeInsert(key.getId(), StorableType(value));
}

template <typename T, typename std::enable_if_t<std::is_base_of<Storable, T>::value, int> = 0>
bool insert(Key<K, T> const& key, T const& value) {
if (this->contains(key.getId())) {
return false;
}

auto holder = value.clone();
return unsafeInsert(key.getId(), StorableType(std::move(holder)));
}

/** @} */

/**
* Insert an element into the map, if the map doesn't already contain a mapping with a conflicting key.
*
Expand Down
38 changes: 2 additions & 36 deletions include/lsst/afw/typehandling/SimpleGenericMap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,26 +56,11 @@ class SimpleGenericMap final : public MutableGenericMap<K> {

public:
SimpleGenericMap() = default;
SimpleGenericMap(SimpleGenericMap const& other) : _storage() {
for (auto const& keyValue : other._storage) {
auto const& key = keyValue.first;
auto const& value = keyValue.second;
_storage.insert(std::make_pair(key, _proxyCopy(value)));
}
}
SimpleGenericMap(SimpleGenericMap const& other) = default;
SimpleGenericMap(SimpleGenericMap&&) = default;
virtual ~SimpleGenericMap() noexcept = default;

SimpleGenericMap& operator=(SimpleGenericMap const& other) {
std::unordered_map<K, StorableType> copy;
for (auto const& keyValue : other._storage) {
auto const& key = keyValue.first;
auto const& value = keyValue.second;
copy.insert(std::make_pair(key, _proxyCopy(value)));
}
std::swap(_storage, copy);
return *this;
};
SimpleGenericMap& operator=(SimpleGenericMap const& other) = default;
SimpleGenericMap& operator=(SimpleGenericMap&&) = default;

typename GenericMap<K>::size_type size() const noexcept override { return _storage.size(); }
Expand Down Expand Up @@ -117,25 +102,6 @@ class SimpleGenericMap final : public MutableGenericMap<K> {
private:
// StorableType is a value, so we might as well use it in the implementation
std::unordered_map<K, StorableType> _storage;

class _ProxyCopier {
public:
template <typename T>
StorableType operator()(T const& value) const {
return value;
}
StorableType operator()(std::unique_ptr<Storable> const& pointer) const { return pointer->clone(); }
};

/**
* Make a copy of any value stored in StorableType.
*
* @param value The value to copy.
* @returns If `value` is a unique_ptr to Storable, returns a unique_ptr
* to a copy of the original value. Otherwise, returns an
* ordinary copy.
*/
StorableType _proxyCopy(StorableType const& value) { return boost::apply_visitor(_ProxyCopier(), value); }
};

} // namespace typehandling
Expand Down
33 changes: 17 additions & 16 deletions python/lsst/afw/typehandling/_GenericMap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,22 @@
#include "lsst/afw/typehandling/GenericMap.h"
#include "lsst/afw/typehandling/python.h"

// From https://pybind11.readthedocs.io/en/stable/advanced/cast/stl.html#c-17-library-containers
// Not needed once we can use std::variant
namespace pybind11 {
namespace detail {
template <typename... Ts>
struct type_caster<boost::variant<Ts...>> : variant_caster<boost::variant<Ts...>> {};
template <>
struct visit_helper<boost::variant> {
template <typename... Args>
static auto call(Args&&... args) -> decltype(boost::apply_visitor(args...)) {
return boost::apply_visitor(args...);
}
};
} // namespace detail
} // namespace pybind11

namespace py = pybind11;
using namespace py::literals;

Expand All @@ -53,25 +69,10 @@ class Publicist : public MutableGenericMap<K> {
using MutableGenericMap<K>::unsafeErase;
};

// This class lets us implement __getitem__ using GenericMap::unsafeLookup
// Nonlocal class definition to let us use a method template for well-behaved types
class VariantOneWayCaster {
public:
template <typename T>
py::object operator()(T const& value) {
return py::cast(value);
}
// GenericMap::get returns the same reference, depend on RVP to avoid double-deletion
py::object operator()(std::unique_ptr<Storable> const& pointer) { return py::cast(*pointer); }
};

template <typename K>
py::object get(GenericMap<K>& self, K const& key) {
auto callable = &Publicist<K>::unsafeLookup;
auto ref = (self.*callable)(key);
// Use visitor because py::type_caster for variant chokes on unique_ptr const&
py::object obj = boost::apply_visitor(VariantOneWayCaster(), ref);
return obj;
return py::cast((self.*callable)(key));
};

template <typename K>
Expand Down

0 comments on commit e9f231f

Please sign in to comment.