moved *InferModel classes to infer.{cpp|h}
#6922
Conversation
|
As suggested by @pfultz2 in #6845 (comment). |
lib/infer.cpp
Outdated
| @@ -390,6 +390,7 @@ std::vector<MathLib::bigint> getMaxValue(const ValuePtr<InferModel>& model, cons | |||
|
|
|||
| namespace { | |||
| struct IntegralInferModel : InferModel { | |||
| private: | |||
There was a problem hiding this comment.
Why is private being added? I don't think it's necessary and it's a little confusing since the interface is public.
There was a problem hiding this comment.
That is so it can only call it through the interface and not on the object it inherits from.
There was a problem hiding this comment.
Why? Its faster calling it directly(there is no vtable lookup).
There was a problem hiding this comment.
In that case - why having an interface?
Also this change is probably pedantic because the callers will never see the actual implementation and only use the interface.
There was a problem hiding this comment.
In that case - why having an interface?
So we can check that the interface is implemented correctly. Also if this is used across TUs we will need the interface, but if its in the same TU we can use a template function and use REQUIRES("T must be an InferModel", std::is_base_of<T, InferModel>) to improve error messages.
There was a problem hiding this comment.
Also if this is used across TUs we will need the interface, but if its in the same TU we can use a template function and use
REQUIRES("T must be an InferModel", std::is_base_of<T, InferModel>)to improve error messages.
Interesting point. I was wondering if there was some cleanup which could be done now they are all in the same place. Will look into it.
There was a problem hiding this comment.
I dropped commit making things private for now.
No description provided.