Fix Issue 4125 - std.numeric.gcd can use a binary GCD by Darredevil · Pull Request #4940 · dlang/phobos

Darredevil · 2016-12-09T15:42:15Z

After conducting some benchmarks we arrived to the conclusion that currently the dmd compiler works best with Euclid's algorithm for GCD, othewise we use Stein's algorithm for ldc or gdc.

https://issues.dlang.org/show_bug.cgi?id=4125

dlang-bot · 2016-12-09T15:42:18Z

Fix	Bugzilla	Description
✓	4125	std.numeric.gcd can use a binary GCD

andralex

few nits

andralex · 2016-12-09T16:22:19Z

std/numeric.d

 /**
 Computes the greatest common divisor of $(D a) and $(D b) by using
-Euclid's algorithm.
+Euclid's or Stein's algorithm depending on compiler.


No need to be this detailed, just write "... using an efficient algorithm such as Euclid or Stein."

Oh, you may also add Web links to the algorithms: "... using an efficient algorithm such as $(HTTP xyz.com/page.html, Euclid) or $(HTTP xyz.com/page.html, Stein)."

Oh, you may also add Web links to the algorithms:

@Darredevil FYI there's also the LINK2 macro which allows to have clickable/copy-pasteable links in the source code. For more information, see e.g. this discussion.

andralex · 2016-12-09T16:22:30Z

std/numeric.d

-    else
-    {
-        static if (T.min < 0)
+    version(DigitalMars) {


brace on its own line

Since the CircleCI style checker has been re-enabled the failing status of CircleCi refers to detected style violations :)
(or of course to build errors).

@Darredevil there's also a style target in the Makefile, so you can e.g. use make -f posix.mak style to run the linting locally.

andralex · 2016-12-09T16:23:32Z

std/numeric.d

+        static if (is(T == const) || is(T == immutable))
        {
-            assert(a >= 0 && b >=0);
+            return gcd!(Unqual!T)(a, b);


This part is common to both implementations, so pull it before the version test.

andralex · 2016-12-09T16:27:09Z

std/numeric.d

+            immutable uint shift = bsf(a | b);
+            a >>= a.bsf;
+
+            do {


git grep ' do$' **/*.d | wc -l 55 git grep ' do *{' **/*.d | wc -l 8

So I guess we want to normalize on { on its own line, too. cc @wilzbach @CyberShadow

So I guess we want to normalize on { on its own line, too

Sure -> #4942

Also note that our "style" CI is already complaining at the moment :)

wilzbach · 2016-12-09T17:30:25Z

std/numeric.d

+            return gcd!(Unqual!T)(a, b);
+        }
+        else {
+            import std.algorithm: swap;


The D style for selective imports is to have a space before and after the colon (:) like import std.range : zip

Darredevil · 2016-12-10T22:31:54Z

@andralex @wilzbach think I addressed everything you mentioned, let me know if there is anything else, thx!

andralex · 2016-12-12T13:34:06Z

std/numeric.d

+        do
+        {
+            b >>= b.bsf;
+


redundant whitespace

andralex · 2016-12-12T13:36:00Z

std/numeric.d

 Computes the greatest common divisor of $(D a) and $(D b) by using
-Euclid's algorithm.
+an efficient algorithm such as $(HTTP https://en.wikipedia.org/wiki/Euclidean_algorithm, Euclid's)
+or $(HTTP https://en.wikipedia.org/wiki/Binary_GCD_algorithm, Stein's) algorithm.


These links will not work. You either use $(HTTP no.protocol.here) or $(LINK2 protocol://site.domain). Right now the link formed will be http://https://en.wikipedia.org/wiki/Euclidean_algorithm, which obviously won't work.

That's exactly why we want to remove the HTTP and HTTPS macros, they cost a lot more than they actually do save!

edit: didn't see that the change was part of the PR

andralex · 2016-12-12T13:56:59Z

Auto-merge toggled on

andralex reviewed Dec 9, 2016

View reviewed changes

wilzbach mentioned this pull request Dec 9, 2016

Enforce Allman style for do { #4942

Merged

wilzbach reviewed Dec 9, 2016

View reviewed changes

Darredevil force-pushed the issue-4125 branch 4 times, most recently from 43899a0 to 840be6a Compare December 10, 2016 21:42

andralex approved these changes Dec 12, 2016

View reviewed changes

Fix Issue 4125 - std.numeric.gcd can use a binary GCD

6b4c258

Darredevil force-pushed the issue-4125 branch from 840be6a to 6b4c258 Compare December 12, 2016 13:39

andralex merged commit 19445fc into dlang:master Dec 12, 2016

wilzbach mentioned this pull request Dec 13, 2016

Fix link format for changes in PR 4940 (std.numeric.gcd) #4951

Closed

ibuclaw mentioned this pull request Apr 17, 2021

std.numeric: Always use Stein's algorithm for integral gcd() #7976

Merged

Uh oh!

Conversation

Darredevil commented Dec 9, 2016

Uh oh!

dlang-bot commented Dec 9, 2016

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

andralex Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Darredevil commented Dec 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Dec 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andralex Dec 9, 2016 •

edited

Loading

wilzbach Dec 13, 2016 •

edited

Loading