Better use of std::enable_if#1177
Conversation
There was a problem hiding this comment.
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.