Fix issue 20924 - std.numeric.gcd cannot be used with const BigInt #7531
Conversation
|
Thanks for your pull request and interest in making D better, @Biotronic! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#7531" |
Biotronic
force-pushed
the
Issue20924
branch
2 times, most recently
from
207e4ef to
516b4dc
Compare
std/numeric.d
Outdated
| if (!isIntegral!T && | ||
| is(typeof(T.init % T.init)) && | ||
| is(typeof(T.init == 0 || T.init > 0))) | ||
| { | ||
| import std.algorithm.mutation : swap; | ||
|
|
||
| Unqual!T a = _a; | ||
| Unqual!T b = _b; | ||
|
|
||
| enum canUseBinaryGcd = is(typeof(() { | ||
| T t, u; |
There was a problem hiding this comment.
This check will still fail for const/immutable T.
std/numeric.d
Outdated
| Unqual!T a = _a; | ||
| Unqual!T b = _b; | ||
|
|
There was a problem hiding this comment.
Can you static if this so you don't make copies if you don't have to? If T is not const, just alias a = _a?
There was a problem hiding this comment.
Other option: use const T for the params and then always make a mutable copy. This would prevent all the 'redundant' const/immutable instantiations, another (common) problem of such public templates. Or just forward to the mutable version as integer gcd above:
Lines 2936 to 2939 in a394c13
There was a problem hiding this comment.
The main problem with the const T idea over the alias or forwarding is it pessimizes all code: the copy will always happen. There's also a theoretical issue of a type T such that const(T) can't be converted to T.
There was a problem hiding this comment.
So you've changed your mind?
std/numeric.d
Outdated
| { | ||
| import std.algorithm.mutation : swap; | ||
| enum canUseBinaryGcd = is(typeof(() { | ||
| Unqual!T t, u; |
There was a problem hiding this comment.
In this branch you've already established is(T == Unqual!T).
| @@ -2987,64 +2987,70 @@ if (isIntegral!T) | |||
| // This overload is for non-builtin numerical types like BigInt or | |||
| // user-defined types. | |||
| /// ditto | |||
| T gcd(T)(T a, T b) | |||
| auto gcd(T)(T a, T b) | |||
There was a problem hiding this comment.
I dislike auto as return type - it requires the user to look at the source to infer it; for the compiler, it means it always has to analyze the function to infer it too (but for templates it doesn't matter that much).
| @@ -2987,64 +2987,70 @@ if (isIntegral!T) | |||
| // This overload is for non-builtin numerical types like BigInt or | |||
| // user-defined types. | |||
| /// ditto | |||
| T gcd(T)(T a, T b) | |||
| auto gcd(T)(T a, T b) | |||
There was a problem hiding this comment.
| auto gcd(T)(T a, T b) | |
| Unqual!T gcd(T)(T a, T b) |
| import std.bigint : BigInt; | ||
| const a = BigInt("123143238472389492934020"); | ||
| const b = BigInt("902380489324729338420924"); | ||
| assert(__traits(compiles, gcd(a, b))); |
There was a problem hiding this comment.
Same test but no runtime checks & better error messages:
| assert(__traits(compiles, gcd(a, b))); | |
| if (false) gcd(a, b); |
n8sh
added
Merge:72h no objection -> merge
No description provided.