Skip to content
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

Merged
merged 3 commits into from
Dec 23, 2024
Merged

Conversation

beinhaerter
Copy link
Contributor

Replace all occurances of class = std::enable_if_t<Cond> and typename = std::enable_if_t<Cond> with std::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.

Copy link
Collaborator

@carsonRadtke carsonRadtke left a 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.

@carsonRadtke carsonRadtke added Type: Bug Indicates a bug or error Status: In Progress Currently being worked on labels Dec 18, 2024
@beinhaerter
Copy link
Contributor Author

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.

@beinhaerter
Copy link
Contributor Author

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 not_null and span were not useful, I removed them.

For the other changes I split my PR into three commits:

  1. add the tests that should fail but that do not fail
  2. do the code changes that make the tests fail as expected
  3. revert all changes from the first commit

I would have preferred to keep the tests from the first commits, but my SFINAE skills are too low. I tried it the way span_compatibility_tests is doing it, but I did not get it working.

Werner Henze added 2 commits December 19, 2024 10:03
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.
@beinhaerter
Copy link
Contributor Author

I am now happy with the PR. I was able to write tests like static_assert(XXCompilesFor<YY>, ""); that work for the code before the fix and where negating the condition like static_assert(!XXCompilesFor<YY>, ""); works after the fix. So we can now keep the tests in place and get rid of the third commit that deleted the tests.

@carsonRadtke carsonRadtke added Status: Review Needed Needs review from GSL maintainers and removed Status: In Progress Currently being worked on labels Dec 19, 2024
@carsonRadtke carsonRadtke self-requested a review December 20, 2024 19:36
@carsonRadtke
Copy link
Collaborator

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.

- 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:
-
@carsonRadtke
Copy link
Collaborator

This looks great! Thanks for making all those changes.

@carsonRadtke carsonRadtke merged commit fcd55ee into microsoft:main Dec 23, 2024
83 checks passed
@carsonRadtke carsonRadtke removed their request for review December 23, 2024 16:41
@carsonRadtke carsonRadtke added Status: Completed Work is done and can be closed and removed Status: Review Needed Needs review from GSL maintainers labels Dec 23, 2024
@beinhaerter beinhaerter deleted the enable_if branch December 24, 2024 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Completed Work is done and can be closed Type: Bug Indicates a bug or error
Projects
Development

Successfully merging this pull request may close these issues.

2 participants