Factor integer traits and disentangle traits headers#1314
Factor integer traits and disentangle traits headers#1314AntoinePrv merged 1 commit intoxtensor-stack:masterfrom
Conversation
AntoinePrv
force-pushed
the
factoring
branch
5 times, most recently
from
8d85e7f to
cc29bd4
Compare
| using signed_type = std::int64_t; | ||
| using unsigned_type = std::uint64_t; | ||
| using floating_point_type = double; | ||
| }; |
| struct widen; | ||
|
|
||
| template <typename T> | ||
| struct widen<T, std::enable_if_t<std::is_floating_point<T>::value>> |
There was a problem hiding this comment.
nit: you could inherit from `detail::sized_num_types::floating_point_type|
There was a problem hiding this comment.
Not sure to understand the benefit of it since we would still need to define type as base::floating_point_type in the struct.
| }; | ||
|
|
||
| template <typename T> | ||
| struct widen<T, std::enable_if_t<!std::is_floating_point<T>::value && std::is_unsigned<T>::value>> |
There was a problem hiding this comment.
!std::is_floating_point seems redundant here
| #include "xtl/xcomplex.hpp" | ||
| #endif | ||
|
|
||
| #include "./xsimd_batch_fwd.hpp" |
serge-sans-paille
left a comment
serge-sans-paille
left a comment
There was a problem hiding this comment.
While you're at it, maybe you could make all internal includes absolute instead of relative?
|
@serge-sans-paille I definitely want to do a pass on header but I think this is PR is ugly enough (and I am already spending too much time on xsimd 😅 ) |
I think we should keep them relativem this makes it easier for projects vendoring xsimd. |
| struct widen; | ||
|
|
||
| template <typename T> | ||
| struct widen<T, std::enable_if_t<std::is_floating_point<T>::value>> |
There was a problem hiding this comment.
Not sure to understand the benefit of it since we would still need to define type as base::floating_point_type in the struct.
| ****************************************************************************/ | ||
|
|
||
| #ifndef XSIMD_TYPE_TRAITS_HPP | ||
| #define XSIMD_TYPE_TRAITS_HPP |
There was a problem hiding this comment.
This file should be named xsimd_type_traits.hpp for consistency, and should probably live in xsimd/types. Only xsimd.hpp should be at the "root" of the project
There was a problem hiding this comment.
This file should be named xsimd_type_traits.hpp for consistency
Ok, though I think we should get rid of that convention in the next major release.
and should probably live in xsimd/types. Only xsimd.hpp should be at the "root" of the project
@JohanMabille
This is weird to me. Why would everything need to live in type? IMHO this participate in the general confusion about header organization and weird "../../type" inclusion.
How about "xsimd/utils"?
There was a problem hiding this comment.
though I think we should get rid of that convention in the next major release.
Not sure to understand why, having a single "include-all" header at the root of the project is a convenient pattern; having many headers at the same level would be weird.
How about "xsimd/utils"?
Works for me, as long as it is not at the sane level as xsimd.hpp
There was a problem hiding this comment.
though I think we should get rid of that convention in the next major release.
Not sure to understand why, having a single "include-all" header at the root of the project is a convenient pattern; having many headers at the same level would be weird.
How about "xsimd/utils"?
Works for me, as long as it is not at the sane level as
xsimd.hpp
I meant the xsimd_ prefix convention for filenames which I think is needlessly verbose and very much C-style.
There was a problem hiding this comment.
We can see to remove it in next major release, but for now let's stay consistent.
I think we should settle on either |
| ****************************************************************************/ | ||
|
|
||
| #ifndef XSIMD_TYPE_TRAITS_HPP | ||
| #define XSIMD_TYPE_TRAITS_HPP |
There was a problem hiding this comment.
We can see to remove it in next major release, but for now let's stay consistent.
3 participants
Factor integer trait at a higher level to avoid repeated work of explicitly defining something for all sizes.
Basically avoid repeating the following kind of logic all over:
This is added in
xsimd/type_traits.hpp.This PR as a lot of noise as I needed to fix a few circular header inclusion.
This PR is still imperfect. IMHO we still have some work work to properly factor utilities and ensure all header inclusion are non circular, order independent, and do not depend on leaking behavior (or transitively dependent).