Adding compiler warnings exposed a potential bug induced by invoking undefined behaviour in the _tensor_basis class to convert a double to its bits. To patch this issue and prevent problems, we replaced the offending reinterpret_cast with a pair of calls to memcpy to copy the bits into an integer temporary and then perform the mask and copy back into a double. Obviously this hides the much more simple nature of this operation, even though the compiler will see through this and optimise away any actual copies.
A much more sensible approach is to define the word type as a union, containing a double and an appropriately sized integer, so that it can be considered as both a double and an integer simultaneously. I've mocked up such a base class in a new branch https://github.com/terrylyons/libalgebra/tree/tensor-key-rework. The key class is as follows:
template<typename Word = word_t>
class tensor_word_internal
{
using integral_type = typename fp_info<Word>::unsigned_int_type;
union
{
Word as_float;
integral_type as_bits;
};
explicit constexpr tensor_word_internal(integral_type bits) : as_bits(bits)
{}
public:
constexpr tensor_word_internal() noexcept : as_float(1.0)
{}
constexpr tensor_word_internal(Word w) noexcept : as_float(w)
{
}
explicit operator Word&() noexcept
{
return as_float;
}
constexpr explicit operator const Word&() const noexcept
{
return as_float;
}
explicit operator integral_type&() noexcept
{
return as_bits;
}
constexpr explicit operator const integral_type&() const noexcept
{
return as_bits;
}
constexpr tensor_word_internal operator&(const integral_type mask) const
{
return tensor_word_internal(as_bits & mask);
}
tensor_word_internal& operator<<(const tensor_word_internal& other)
{
auto dPowerOfTwo = other & fp_info<Word>::mantissa_mask_zeroes;
as_float = as_float * dPowerOfTwo.as_float + other.as_float - dPowerOfTwo.as_float;
return *this;
}
bool operator==(const tensor_word_internal& rhs) const
{
return as_float == rhs.as_float;
}
bool operator!=(const tensor_word_internal& rhs) const
{
return !(rhs == *this);
}
bool operator<(const tensor_word_internal& rhs) const
{
return as_float < rhs.as_float;
}
bool operator>(const tensor_word_internal& rhs) const
{
return rhs < *this;
}
bool operator<=(const tensor_word_internal& rhs) const
{
return !(rhs < *this);
}
bool operator>=(const tensor_word_internal& rhs) const
{
return !(*this < rhs);
}
};I've made most of the functionality constexpr, so the compiler can evaluate certain things at compile-time. This is not supposed to be the final for, I just wanted something that I could substitute in to the existing _tensor_basis class to test with minimal changes. In reality, all of this functionality can be implemented directly in _tensor_basis to avoid lots of repetition.
While we're making changes to _tensor_basis, I suggest we change the name of the class to avoid leading underscores, since these are typically reserved for compiler built-ins. Moreover, I suggest we remove the Depth parameter since it isn't really needed. We can handle the overflowing cases at a higher level with much greater efficiency, so having a template parameter just leads to instantiating more templates than necessary.
Finally, we can introduce a template parameter (with default value double) to control the floating point storage type, rather than relying on a typedef in the library. This will allow external users to select a float rather than a double for the storage.
Adding compiler warnings exposed a potential bug induced by invoking undefined behaviour in the
_tensor_basisclass to convert a double to its bits. To patch this issue and prevent problems, we replaced the offendingreinterpret_castwith a pair of calls tomemcpyto copy the bits into an integer temporary and then perform the mask and copy back into a double. Obviously this hides the much more simple nature of this operation, even though the compiler will see through this and optimise away any actual copies.A much more sensible approach is to define the word type as a union, containing a double and an appropriately sized integer, so that it can be considered as both a double and an integer simultaneously. I've mocked up such a base class in a new branch https://github.com/terrylyons/libalgebra/tree/tensor-key-rework. The key class is as follows:
I've made most of the functionality constexpr, so the compiler can evaluate certain things at compile-time. This is not supposed to be the final for, I just wanted something that I could substitute in to the existing
_tensor_basisclass to test with minimal changes. In reality, all of this functionality can be implemented directly in_tensor_basisto avoid lots of repetition.While we're making changes to
_tensor_basis, I suggest we change the name of the class to avoid leading underscores, since these are typically reserved for compiler built-ins. Moreover, I suggest we remove theDepthparameter since it isn't really needed. We can handle the overflowing cases at a higher level with much greater efficiency, so having a template parameter just leads to instantiating more templates than necessary.Finally, we can introduce a template parameter (with default value double) to control the floating point storage type, rather than relying on a typedef in the library. This will allow external users to select a float rather than a double for the storage.