-
Notifications
You must be signed in to change notification settings - Fork 743
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
Better use of std::enable_if #1177
Conversation
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 had originally considered asking for this change as part of #1174, but I was worried there may be subtle differences that need to be considered for each change.
From my point of view, I would like justification for each change (i.e. an example of accepted code that shouldn't be accepted). If the std::enable_if_t
is impossible to override, I don't think the change is necessary. That being said, I will bring this PR up during the monthly maintainers sync (which happens to be this evening) to get a second opinion.
Sounds good. I am not a template expert. I just saw the original PR and player around a bit myself. I have not tested if every changed occurance was wrong/bad. |
c3ba8d0
to
3d8a277
Compare
I did some investigation. It seems not be possible to explicitely choose the template instantiation for a templated constructor in a class template, so my changes for For the other changes I split my PR into three commits:
I would have preferred to keep the tests from the first commits, but my SFINAE skills are too low. I tried it the way |
Replace the occurances of `class = std::enable_if_t<Cond>` and `typename = std::enable_if_t<Cond>` that have been identified in the previous commit with `std::enable_if_t<Cond, bool> = true`. This commit is inspired by microsoft#1174, which changed one occurance in the owner header. This commit is aimed to fix all remaining occurances.
3d8a277
to
0dc891b
Compare
I am now happy with the PR. I was able to write tests like |
Thanks for adding examples and it is awesome that they are now part of the tests. I will take another look once the tests are green for all compilers. FYI - I made a change in the repo settings; the actions should now automatically run for your PRs. |
49ca02f
to
14b4a0e
Compare
- core.cxx_gsl aktualisiert auf [](https://gitlab.avm.de/fos/repos/core.cxx_gsl/-/commit/) - plc.access_lib aktualisiert auf [](https://gitlab.avm.de/fos/repos/plc.access_lib/-/commit/) - plc.common aktualisiert auf [](https://gitlab.avm.de/fos/repos/plc.common/-/commit/) - plc.daemon aktualisiert auf [](https://gitlab.avm.de/fos/repos/plc.daemon/-/commit/) - Test Plan: -
893ca34
to
321a9a3
Compare
This looks great! Thanks for making all those changes. |
Replace all occurances of
class = std::enable_if_t<Cond>
andtypename = std::enable_if_t<Cond>
withstd::enable_if_t<Cond, bool> = true
.This commit is inspired by #1174, which changed one occurance in the owner header. This commit is aimed to fix all remaining occurances.