[TypeTraits] Move general-purpose metaprogramming utilities from TDataFrame's utils to ROOT TypeTraits #636
Conversation
|
Starting build on |
1 similar comment
|
Starting build on |
|
A google test is on the way |
|
Build failed on slc6/gcc62. Warnings:
Failing tests: |
|
Build failed on slc6/gcc49. Warnings:
Failing tests: |
|
Build failed on mac1012/native. Warnings:
Failing tests: |
|
Build failed on ubuntu14/native. Warnings:
Failing tests: |
|
Build failed on centos7/gcc49. Warnings:
Failing tests: |
| *************************************************************************/ | ||
|
|
||
| #ifndef ROOT_TTypeTraits | ||
| #define ROOT_TTypeTraits |
There was a problem hiding this comment.
Isn't this supposed to drop the extra T as well?
There was a problem hiding this comment.
good catch, thanks!
|
Starting build on |
|
A google test has been added in |
|
Build failed on mac1012/native. Warnings:
Failing tests: |
|
Build failed on slc6/gcc62. Warnings:
Failing tests: |
|
Build failed on ubuntu14/native. Warnings:
Failing tests: |
|
Build failed on slc6/gcc49. Warnings:
Failing tests: |
|
Build failed on centos7/gcc49. Warnings:
Failing tests: |
|
@phsft-bot build please. |
|
Starting build on |
| }; | ||
|
|
||
| template <int D, typename P, template <int, typename, template <typename> class> class... S> | ||
| struct IsV7Hist<ROOT::Experimental::THist<D, P, S...>> : public std::true_type { |
There was a problem hiding this comment.
Could we move these two back to an internal file, it is not quite 'general' enough :) [ i.e. ROOT implementation detail]
There was a problem hiding this comment.
we don't want ROOT type traits in ROOT/TypeTraits? :) note that it does not introduce a dependency on libHisto, it just needs a forward decl
There was a problem hiding this comment.
moved back to TDataFrame
| /// e.g. TakeFirstParameter<U<A,B>> is A | ||
| /// TakeFirstParameter<T> is T | ||
| template <typename T> | ||
| struct TakeFirstParameter { |
There was a problem hiding this comment.
What is the difference between TakeFirst and TakeFirstParameter? Do we need both? Can we distinguish them more?
There was a problem hiding this comment.
TakeFirst acts on variadic lists of types, e.g. TakeFirst_t<A,B> is A. TakeFirstParameter acts on the parameter list of a template object, e.g. TakeFirstParameter_t<U<A,B>> is A. They do different things, so I would say we "need" them both. I'm open to suggestions regarding the names (TakeFirstType and TakeFirstParameter?). I thought the docs were clear but maybe not?
There was a problem hiding this comment.
renamed to TakeFirstType
There was a problem hiding this comment.
I suppose your renamed the first one (formely TakeFirst), in which case, this is indeed better. thanks
|
Build failed on mac1012/native. Warnings:
Failing tests: |
| @@ -0,0 +1,143 @@ | |||
| #include "ROOT/TypeTraits.hxx" | |||
There was a problem hiding this comment.
Can we please rename the test file either TypeTraits.test.cxx (or testTypeTrait.cxx) to distinguish it (beyhond just the directory) from an implementation file? Thanks.
There was a problem hiding this comment.
We were using EntityToTestTests.cxx pattern. Having something/test/test*.cxx looks very weird...
There was a problem hiding this comment.
Who's "we"? ls */*/test/*.cxx seems to disagree...
|
Starting build on |
Axel-Naumann
left a comment
Axel-Naumann
left a comment
There was a problem hiding this comment.
Generally, please consider moving declarations into Detail:: or even Internal::: the latter allows us to change these declarations without notice.
| using Test_t = typename std::decay<T>::type; | ||
|
|
||
| template <typename A> | ||
| static constexpr bool Test(A *pt, A const *cpt = nullptr, decltype(pt->begin()) * = nullptr, |
There was a problem hiding this comment.
I'd have written that as
template<class A,
class NonConstA = typename std::remove_const<A>::type,
class ConstA = typename std::add_const<A>::type>
Test(NonConstA::iterator = {},
ConstA::const_iterator = {},
NonConstA::iterator = decltype(NonConstA().begin())()
NonConstA::iterator = decltype(NonConstA().end())(),
ConstA::const_iterator = decltype(ConstA().begin())(),
ConstA::const_iterator = decltype(ConstA().end())(),
bool = typename std::is_same<delctype(*ConstA::const_iterator()), typename add_const<ConstA::value_type>::type>::value,
bool = typename std::is_same<delctype(*NonConstA::iterator()), ConstA::value_type>::value)
This ensures that
- the container and
(const_)iteratorare default constructible - there is a
(const_)iteratorthat matches the return types ofbegin()andend(), const and non-const versions, (const_)iteratorcan be derefed giving(const) value_type
I see you have more tests in the body. Note that e.g. **piwill trigger a compilation failure if iterator has no deref, instead of returning false, which is why I added checks to the signature. More might be needed.
There was a problem hiding this comment.
Thanks for looking into this. This was implemented by @dpiparo so I would ask his opinion too.
There was a problem hiding this comment.
Hi @bluehood , @Axel-Naumann I am of course fine with the new implementation!
|
|
||
| /// Check whether a histogram type is a classic or v7 histogram. | ||
| template <typename T> | ||
| struct IsV7Hist : public std::false_type { |
There was a problem hiding this comment.
I don't think we need this in the public part of the type traits. Maybe at least ROOT::Detail:: if not ROOT::Internal? Do you think we'll have more uses?
There was a problem hiding this comment.
@pcanal expressed the same concern. I'm outnumbered, so I'll move IsV7Hist back inside TDataFrame's stuff.
| }; | ||
|
|
||
| /// Extract types from the signature of a callable object. | ||
| template <typename T> |
There was a problem hiding this comment.
I would make the generic one empty. Then have a specialization for those T that have a call operator.
| struct CallableTraits { | ||
| using arg_types = typename CallableTraits<decltype(&T::operator())>::arg_types; | ||
| using arg_types_nodecay = typename CallableTraits<decltype(&T::operator())>::arg_types_nodecay; | ||
| using ret_type = typename CallableTraits<decltype(&T::operator())>::ret_type; |
There was a problem hiding this comment.
Can you spell the result (please use the std wording here) type using result_of and invoke_result, for all cases, moving the arg_types deduction into a helper detail struct that you then partially specialize?
There was a problem hiding this comment.
Let me rephrase:
struct foo{
int operator()(int);
double operator()(double);
};
using res = typename CallableTraits<foo(float)>::ret_type;
should res be double or should this be ill-formed?
There was a problem hiding this comment.
What is the standard wording? result_of and invoke_result contain a type, but I think it would be a confusing name here? boost uses return_type in their function_traits, I can change it to that if you prefer.
Your example should be ill-formed because CallableTraits does not require to specify the argument types, users just pass in a callable object. Supporting the syntax you used might be a nice addition but we have no use for it, at least for now.
There was a problem hiding this comment.
Forget what I said - you test the callable, not the expression. Return type is just fine.
|
|
||
| // mutable lambdas and functor classes | ||
| template <typename R, typename T, typename... Args> | ||
| struct CallableTraits<R (T::*)(Args...)> { |
There was a problem hiding this comment.
You allow for non-member function references but not for member function references. Intentional?
There was a problem hiding this comment.
Can you give me an example?
This works:
struct A { void foo() {} };
static_assert(std::is_same<CallableTraits<decltype(&A::foo)>::ret_type, void>::value, "");
There was a problem hiding this comment.
Does static_assert(std::is_same<CallableTraits<decltype(A::foo)>::ret_type, void>::value, ""); work?
There was a problem hiding this comment.
It does not. Can you ever use decltype(A::foo) anywhere?
There was a problem hiding this comment.
Indeed, thanks! Forget this comment, too! :-)
| /// Return first of possibly many template parameters. | ||
| /// For non-template types, the result is the type itself. | ||
| /// e.g. TakeFirstParameter<U<A,B>> is A | ||
| /// TakeFirstParameter<T> is T |
There was a problem hiding this comment.
That is a very ... rare ... behavior. Providing this as a general tool might see misuses / misunderstanding of the interface. I'd naively expect that TakeFirstParameter<T> is either empty or ill-formed, instead of mixing template levels... Do you expect generic usage of this? (Do you really know you need this behavior in your use case? I didn't look at it, it seems surprising...)
There was a problem hiding this comment.
I agree, I will change the behaviour
|
|
||
| /// Compile-time integer sequence generator | ||
| /// e.g. calling GenStaticSeq<3>::type() instantiates a StaticSeq<0,1,2> | ||
| // TODO deprecate as soon as std::index_sequence is backported |
There was a problem hiding this comment.
Can we move this into Experimental::? Then we can just remove it without deprecation, and for now, no other interfaces should be using it.
There was a problem hiding this comment.
I am never sure when it comes to ROOT namespaces. Do you mean ROOT::Experimental:: or ROOT::TypeTraits::Experimental:: or ROOT::Experimental::TypeTraits::?
There was a problem hiding this comment.
Actually since it is not meant to be used by users (std::index_sequence is), I would put it in ::ROOT::Internal (and move it back to DataFrame unless there is another 'current' user ...
There was a problem hiding this comment.
I'll move it back and push for having a backport of std::index_sequence soon :)
|
Build failed on mac1012/native. Warnings:
Failing tests: |
|
Starting build on |
|
Build failed on centos7/gcc49. Errors:
Warnings:
|
|
Build failed on mac1012/native. Warnings:
Failing tests: |
|
Starting build on |
|
Rebased and fixed clang-formatting. There are no items pending afaik. |
It substitutes (and augments) test_functiontraits.
|
Rebased on master, resolved conflicts |
7 participants
Here it is...any kind of feedback welcome :)