CRTP: Traits #84
CRTP: Traits #84
Conversation
| */ | ||
| template <class T> struct delay_static_assert : std::false_type {}; | ||
| template <typename X> class is_complete { |
There was a problem hiding this comment.
I prefer this style of type traits, using partial template specialization:
template<typename T, typename = void>
struct is_complete : std::false_type { };
template<typename T>
struct is_complete<T, typename std::enable_if<sizeof(T) != 0>::type> : std::true_type { };
It's the style used in most of Boost's type trait library, and is easier to read to my eye.
There was a problem hiding this comment.
Yeah I see what you're saying. For a lot of these traits (particularly the first ones I wrote) I started with something I found in cereal, Eigen or stackoverflow then beat on them until they worked for our case ... suppose that explains the mish mash of styles. I'll give converting to the Boost style a shot.
There was a problem hiding this comment.
An alternative would be to change the boost-ish style traits to be more inline with the rest of what albatross does. Consistency in style is important, the actual style doesn't really matter. 😃
Don't take this the wrong way. You already have to bend your mind a bit to grok albatross, using a bit different style in your traits isn't going to make it that much more difficult.
There was a problem hiding this comment.
Yep, I'm with you. I was able to get everything except fit_type to follow the non-boost style. If you can think of a way to switch that over I'm happy to go that route temporarily.
| static constexpr bool value = decltype(test<X>(0))::value; | ||
| template <typename ModelType, typename FeatureType, int = 0> | ||
| struct fit_type { | ||
| typedef void type; |
There was a problem hiding this comment.
Do you need the FeatureType template parameter?
There was a problem hiding this comment.
Yeah, the third specialization of fit_type requires it. It also sort of makes sense for the application since we're asking which fit type would be returned for a model fit with a certain feature type.
| std::string get_name() { | ||
| static_assert(std::is_same<DummyType, Derived>::value, | ||
| "never do covariance_function.get_name<T>()"); | ||
| return typeid(Derived).name(); |
There was a problem hiding this comment.
Just FYI using typeid isn't a compile time thing, it'll interrogate the object at run time. Using RTTI has additional impacts on the size of the compiled code. I'm not saying it shouldn't be used, just wanting to make sure you're aware of the implications. It's probably my embedded background that makes this stand out to me.
There was a problem hiding this comment.
Yeah I think I'm ok with that in this case since these methods really aren't used much, more for convenience when debugging. If it concerns you having it in here though I'm happy to remove the default get_name() which uses typeid and force a user implementing a model to define std::string name() const.
There was a problem hiding this comment.
No reason to get rid of it. In the past I've usually disabled RTTI, but that's because the projects have been trying to shave off every last byte they could from the executable. I don't think we're anywhere near needing to shrink the executable size in Orion.
| struct MultiInheritCallImpl : public U, public BaseWithPublicCallImpl {}; | ||
| } | ||
|
|
||
| template <typename U> class has_any_call_impl { |
There was a problem hiding this comment.
This is a really clever way to check for private call_impl_ as well!
There was a problem hiding this comment.
Heh, yeah a while back I got bit pretty hard with name hiding ... then realized it could be useful.
| } | ||
|
|
||
| TEST(test_traits_core, test_fit_type) { | ||
| EXPECT_TRUE(bool(std::is_same<Z, fit_type<FitModel<X, Z>, Y>::type>::value)); |
There was a problem hiding this comment.
Some failure cases would be good here.
There was a problem hiding this comment.
Currently calling fit_type<Model, Feature> for a model which doesn't have a valid fit for that feature results in a compile error which, I think, is desirable but makes testing failure modes difficult.
Speaking of which, are you aware of any ways of unit testing for situations that you want to make sure don't compile?
| TEST(test_traits_core, test_adaptable_fit_type) { | ||
| EXPECT_TRUE(bool(std::is_base_of<Fit<Adaptable<Extended>, X>, | ||
| fit_type<Extended, Y>::type>::value)); | ||
| EXPECT_TRUE(bool(std::is_base_of<Fit<Adaptable<Extended>, X>, |
There was a problem hiding this comment.
Some failure cases would be good here.
There was a problem hiding this comment.
same as above: I can't set up failure cases for these since inspecting something without a fit_type gives compile error. I think that's OK though.
tests/test_traits_core.cc
Outdated
| bool(has_valid_predict_marginal<HasMarginalPredictImpl, X, Fit<HasMarginalPredictImpl>>::value)); | ||
| EXPECT_TRUE(bool(has_valid_predict_joint<HasJointPredictImpl, X, Fit<HasJointPredictImpl>>::value)); | ||
| EXPECT_TRUE(bool(has_valid_predict_mean<HasAllPredictImpls, X, Fit<HasAllPredictImpls>>::value)); | ||
| EXPECT_TRUE(bool(has_valid_predict_marginal<HasAllPredictImpls, X, Fit<HasAllPredictImpls>>::value)); |
There was a problem hiding this comment.
Some failure cases here would be good.
albatross/cereal/traits.h
Outdated
| namespace albatross { | ||
|
|
||
| /* | ||
| * This little trick was borrowed from cereal, you an think of it as |
This change breaks up
traits.hinto several application specific traits files:core/traits.h,covariance_functions/traits.handcereal/traits.h. And breaks uptest_traits.ccrespectively. The bulk of this change is a result of the moves and adding tests, butcore/traits.hhas changed substantially.Namely, I've added the following traits:
first_template_param_is_base_of,is_valid_fit_type,has_valid_fit,has_possible_fit,fit_model_type,fit_typeandhas_valid_predict.These are already being used in
ModelBase,FitModelandPredictions, look there for example usage.