Upstream some LDC patches #5902

kinke · 2017-12-01T22:28:32Z

Pinging @klickverbot and @ibuclaw.

dlang-bot · 2017-12-01T22:28:33Z

Thanks for your pull request, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

MetaLang · 2017-12-02T03:42:06Z

std/digest/murmurhash.d

        assert(bufferSize < Element.sizeof);

-        // Check if we have some leftover data in the buffer. Then fill the first block buffer.
+        // Check if we don't even fill up a single block buffer.


This is very awkward wording. What about "check if we can fill up a single block buffer"?

The true-branch with early return is for the opposite case, i.e., where the current bytes in the block buffer and the new bytes to be hashed don't add up to a full Element size.

Not sure whether applicable here, but don't forget that for simple documentation matters patching someone else's PR is almost as easy as submitting a comment.

I don't like the idea of editing someone's pull request unless it's something small and obviously wrong or it's necessary for some reason.

@kinke "check if we can 't fill up a single block buffer?" ;-)

Now: Check if we don't fill up a full block buffer.

Just noticed fill up a full ... =>Check if we don't fill up a whole block buffer.

ibuclaw · 2017-12-09T22:37:59Z

std/digest/murmurhash.d

        const numElements = data.length / Element.sizeof;
        const remainderStart = numElements * Element.sizeof;
-        foreach (ref const Element block; cast(const(Element[]))(data[0 .. remainderStart]))
+        version (haveUnalignedLoads)


Haven't we already talked about this before? Or am I thinking of another patch.

Possibly, not with me though; a first incarnation of this murmurhash patch is in #5358 (which I only realized after creating this PR). This one here still isn't fully satisfactory as casting a static array to another static array type may not be well-defined in the spec (haven't checked, but it's reinterpret_cast vs. conversion to new rvalue). The latest (LDC-specific) version without ambiguities is ldc-developers#44.

Edit: Now updated here too.

Now in PascalCase.

ibuclaw · 2017-12-09T22:39:37Z

std/math.d

        const real zz = z * z;

-        if (zz > 1.0e-20L)
+        enum zzThreshold = (floatTraits!real.realFormat == RealFormat.ieeeDouble ? 1.0e-14L : 1.0e-20L);


Store floatTraits!real in an alias instead of instantiating it multiple times in the same function?

Twice to be precise, and an enum isRealDouble would suffice. If you insist on adding a new line for shortening these 2 lines, I won't refuse. ;)

There was a third usage I previously overlooked => enum realFormat.

joakim-noah · 2017-12-22T14:10:35Z

@kinke, you can remove the second and third commits relating to Android and std.allocator, as they were just merged in another pull.

kinke · 2018-01-04T21:34:50Z

Rebased and slightly reworked.

joakim-noah · 2018-01-05T02:40:31Z

std/datetime/timezone.d

+        /++
+            The default directory where the TZ Database files are. It's empty
+            for Windows, since Windows doesn't have them.
+          +/


Why does this comment need to be copy-pasted for every platform?

AFAICT just because the version(TZDatabaseDir) branch (which seems obsolete) doesn't start with the enum, but needs a prior import; otherwise, the comment could be placed once before the version branches. [Outside the scope of this PR though.]

It was added a week ago: #5966

FWIW we only support building the docs on Posix since 1.5 years, so adding comments in other branches isn't needed, but doesn't hurt either.
Anyhow @kinke said, it doesn't block this PR.

The ddoc comment really should be put in a StdDdoc version block and removed from the other versions.

I see no reason to put this off for later, I'd rather it were at the top with a StdDdoc version or just as a comment. Just change it to say that Windows doesn't have one and TZDatabaseDir lets you set one at compile-time in an external file, which btw it doesn't even explain now.

Who can take care of this in a follow up?

kinke · 2018-01-24T19:10:58Z

Ping - the changes are trivial except for std.digest.murmurhash, but DMD and x86(_64) are mostly unaffected by that commit.

andralex · 2018-01-24T19:52:12Z

std/datetime/timezone.d

+        /++
+            The default directory where the TZ Database files are. It's empty
+            for Windows, since Windows doesn't have them.
+          +/


Who can take care of this in a follow up?

andralex · 2018-01-24T19:54:58Z

@kinke autotester plz

wilzbach · 2018-01-24T19:56:24Z

The failure is unrelated - see #6061

joakim-noah · 2018-01-25T04:17:56Z

I'll fix the comments in another pull, if that's what you're asking.

…oads

Minor fixes after cross-checking against the Cephes implementation for doubles, see https://github.com/jeremybarnes/cephes/blob/master/cmath/tan.c. The `assert(!isNaN(tan(0x1p300L)))` unittest (still) fails for 64-bit reals, while `tan(-0x1p300L) == -0x1p300L`. Not really suprising, as the total loss threshold is 2^30; Cephes returns 0 as error for greater (absolute) inputs. The threshold for 128-bit quadruple reals is 2^55 btw.

quickfur · 2018-01-26T19:24:41Z

Are @MetaLang's concerns already addressed? Should we merge?

joakim-noah · 2018-01-27T03:41:55Z

I think so. It was just about a comment, which has been changed since. Can probably dismiss his review if he doesn't.

MetaLang · 2018-01-29T17:30:59Z

I still don't like the wording (check if we don't is very awkward to read and try to understand), but if this is ready to go it's not worth holding up for a comment.

.

quickfur

OK then, let's merge! It's long overdue.

Fix some comments from #5902

kinke requested review from CyberShadow, PetarKirov, andralex, dnadlinger, ibuclaw, jmdavis and wilzbach as code owners December 1, 2017 22:28

kinke force-pushed the ldc_to_upstream branch from b77ca3d to 6759aa3 Compare December 1, 2017 22:41

kinke referenced this pull request in ldc-developers/phobos Dec 1, 2017

Clean up LDC-specific std.digest.murmurhash extension

4057efb

MetaLang previously requested changes Dec 2, 2017

View reviewed changes

ibuclaw reviewed Dec 9, 2017

View reviewed changes

kinke force-pushed the ldc_to_upstream branch from 6759aa3 to a450d85 Compare December 10, 2017 00:33

ibuclaw self-assigned this Dec 27, 2017

dlang-bot added Merge:Needs Rebase Review:Needs Work labels Dec 29, 2017

Solaris fixes for std.datetime

9f8482a

kinke force-pushed the ldc_to_upstream branch from a450d85 to 3362fb8 Compare January 4, 2018 21:32

dlang-bot removed Merge:Needs Rebase Review:Needs Work labels Jan 4, 2018

joakim-noah reviewed Jan 5, 2018

View reviewed changes

andralex approved these changes Jan 24, 2018

View reviewed changes

std.digest.murmurhash: Support architectures not allowing unaligned l…

d5f710c

…oads

kinke added 2 commits January 26, 2018 20:16

std.math: Improve support for AArch64, MIPS and MIPS64

a32a8ae

kinke force-pushed the ldc_to_upstream branch from 3362fb8 to 31b8fee Compare January 26, 2018 19:17

quickfur approved these changes Jan 29, 2018

View reviewed changes

quickfur added the Merge:auto-merge label Jan 29, 2018

dlang-bot merged commit 2f25cdf into dlang:master Jan 29, 2018

kinke deleted the ldc_to_upstream branch January 29, 2018 21:19

wilzbach added a commit that referenced this pull request Jan 31, 2018

Merge pull request #6095 from joakim-noah/comment

12e227d

Fix some comments from #5902

Uh oh!

Upstream some LDC patches #5902

Upstream some LDC patches #5902

Uh oh!

Conversation

kinke commented Dec 1, 2017

Uh oh!

dlang-bot commented Dec 1, 2017

Bugzilla references

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!

MetaLang Dec 2, 2017 • 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!

kinke Dec 9, 2017 • 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!

joakim-noah commented Dec 22, 2017

Uh oh!

kinke commented Jan 4, 2018

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!

joakim-noah Jan 5, 2018 • 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!

kinke commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Jan 24, 2018

Uh oh!

wilzbach commented Jan 24, 2018

Uh oh!

joakim-noah commented Jan 25, 2018

Uh oh!

quickfur commented Jan 26, 2018

Uh oh!

joakim-noah commented Jan 27, 2018

Uh oh!

MetaLang commented Jan 29, 2018

Uh oh!

quickfur left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

MetaLang Dec 2, 2017 •

edited

Loading

kinke Dec 9, 2017 •

edited

Loading

joakim-noah Jan 5, 2018 •

edited

Loading