Add class template argument deduction guides for distributions #756
Conversation
|
Putzing around in Godbolt, it looks like GCC-8 supports pre-final C++17 deduction guides ( |
|
I'm not sure what's going on with |
|
Well, clang 6-8 are now unhappy with the "no arg" case. These use the default template arg, not the deduction guide, right? So I think we can just get rid of those tests. I believe I've got some of the CI running locally, and removing the "no arg" tests seems to be working. |
jamesfolberth
force-pushed
the
issue754
branch
from
7b09b2c
to
f26c3cb
Compare
|
@jamesfolberth I approved your PR to run against our CI so you should start seeing results. Edit: You don't need to add the platform tags unless there is one specific one you need. The system can't process multiple tags so the default is run everything. |
…g#754 GCC-8 and clang 6-8 were unhappy with the no-arg cases, which use the default template arg, not the deduction guide, anyway.
|
Whoops! That broke it, sorry. I was trying to avoid MSVC, which appeared to me to be failing over the weekend for reasons unrelated to this PR. I'll remove those tags from the commit message. Thanks for your help! |
jamesfolberth
force-pushed
the
issue754
branch
from
f26c3cb
to
14ed931
Compare
|
This PR looks good to me. Do you have anything else to add, and does it solve your problem? |
|
Looks good to me as well. Might want to let @jzmaddock weigh in though; I've pwned myself with argument deduction subtleties before . . . |
|
This was all I had planned for this PR. It does indeed solve the problem I
originally encountered.
Thanks for taking a look. Getting a few of the older compilers to be happy
with the test was a bit delicate.
…On Thu, Feb 10, 2022 at 9:53 AM Nick ***@***.***> wrote:
Looks good to me as well. Might want to let @jzmaddock
<https://github.com/jzmaddock> weigh in though; I've pwned myself with
argument deduction subtleties before . . .
—
Reply to this email directly, view it on GitHub
<#756 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAY4FGFNRVCXCMI4VEZE2ZDU2PUQTANCNFSM5NUUTMSQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Other than the single comment on the fisher_f this looks good to go to me - many thanks to @jamesfolberth for seeing this through! |
|
Oh dang nice catch! Thanks all for the help. Happy to contribute! |
|
FWIW looks like CI is failing waiting on a runner. |
|
Ugh, that's just silly. Anyone any objections to merging anyway? All looks good to me. |
|
I've had the runner farm go on strike a few times. I'm not exactly sure what causes that. Short of stopping and restarting the entire run on GHA that run is toasted. So unless we restart entirely, which seems uwaranted, content gets the nod. |
|
I think we're safe, so I've no problem with merging anyway. After my 3rd commit (6bd06cf) I ran locally |
Fixes #754 by adding deduction guides for the case where the class template arguments are not given explicitly.
Follows the demonstration by @jzmaddock.
I didn't put in
static_assertfor silly cases, e.g., where the user writeschi_squared_distribution<int>.If that's what they write, so be it.