-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allowing partial specialization of the convert struct #922
base: master
Are you sure you want to change the base?
Conversation
How should I approach the testing of this? It's easy to check that it works for the correct subset of types but not that it doesn't for the others since it would create a compilation error |
5d7de05
to
37af483
Compare
I added a small test to make sure that the feature works and to potentially avoid future regressions. Let me know if you require more testing |
@jbeder Could you review this PR when you have time? |
class B : public A { | ||
public: | ||
// override virtual load/emit methods | ||
virtual void load(const YAML::Node &node) override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it's a test case, I think the constructors are needed, like #310. It is also convenient for later usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about updating the Tutorial.md at the same time. And use this in testcase.
A a(1);
B b(1, 2);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the test cases to use the constructors and added them in the tutorial.
However the tutorial doesn't make use of them
37af483
to
0085ebf
Compare
89b6ad1
to
f9b25df
Compare
I am also interested in getting this merged, was actually about to submit the same merge request myself. This change would facilitate using Stackoverflow discussion that discusses the issue of not being able to use SFINAE without a second template paramter. |
Is there a plan for getting this merged. I am also interested in this functionality. |
This is done by adding a second, defaulted, template parameter that can be used in conjunction of std::enable_if An example of usage is added to the tutorial
f9b25df
to
3be2e1b
Compare
Is there any hope to see this merged eventually? It would be a life saver |
This is done by adding a second, defaulted, template parameter that can
be used in conjunction of std::enable_if
An example of usage is added to the tutorial
The code and example has been taken from #310