Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(138)

Issue 2102223005: [builtins] Migrate Math.hypot() to C++ builtins. (Closed)

Created:
4 years, 5 months ago by Franzi
Modified:
4 years, 5 months ago
Reviewers:
Benedikt Meurer
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@MathAbs
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[builtins] Migrate Math.hypot() to C++ builtins. Migrate Math.hypot() from JS to C++ builtins. Use normalization and Kahan summation to avoid overflow and rounding errors. R=bmeurer@chromium.org BUG=v8:5165, v8:5086 LOG=n Committed: https://crrev.com/fe1a07fe9e6eb953418dcf2f8b751e9ca5894d42 Cr-Commit-Position: refs/heads/master@{#37473}

Patch Set 1 #

Patch Set 2 : Fix behavior of early return on INFINITY #

Total comments: 1

Patch Set 3 : Do not add Math.hypot() to list of optimized functions. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -30 lines) Patch
M src/bootstrapper.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/builtins.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/builtins.cc View 1 1 chunk +52 lines, -0 lines 0 comments Download
M src/js/math.js View 2 chunks +1 line, -30 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 8 (3 generated)
Franzi
Hi Benedikt, Here's Math.hypot() as C++ Builtin(). Please have a look. Thanks, Franzi
4 years, 5 months ago (2016-06-30 16:44:25 UTC) #1
Benedikt Meurer
LGTM once comment is addressed. Thanks. https://codereview.chromium.org/2102223005/diff/20001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2102223005/diff/20001/src/objects.h#newcode6741 src/objects.h:6741: V(Math, hypot, MathHypot) ...
4 years, 5 months ago (2016-07-01 03:41:20 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2102223005/40001
4 years, 5 months ago (2016-07-01 07:47:08 UTC) #5
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 5 months ago (2016-07-01 08:13:40 UTC) #6
commit-bot: I haz the power
4 years, 5 months ago (2016-07-01 08:15:20 UTC) #8
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/fe1a07fe9e6eb953418dcf2f8b751e9ca5894d42
Cr-Commit-Position: refs/heads/master@{#37473}

Powered by Google App Engine
This is Rietveld 408576698