Skip to content
This repository has been archived by the owner on Jun 5, 2022. It is now read-only.

Math.imul #18

Merged
merged 1 commit into from Oct 17, 2020
Merged

Math.imul #18

merged 1 commit into from Oct 17, 2020

Conversation

matthewleon
Copy link
Contributor

No description provided.

src/Math.js Outdated
};

// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/imul
function emulatedImul(a) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't line 27 ensure that it will be used in environments like Internet Explorer that don't have native imul?

Copy link
Contributor Author

@matthewleon matthewleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed up some of the jshint complaints on this... But the ones regarding the bitwise operations seem strange to me. I'll try add a comment in to disable those.

src/Math.js Outdated
};

// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/imul
function emulatedImul(a) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't line 27 ensure that it will be used in environments like Internet Explorer that don't have native imul?

@paf31
Copy link
Contributor

paf31 commented Nov 20, 2017

Sorry, somehow I missed where your emulated function was used. I don't know how well it will play with DCE though, we might need to combine them.

I'm not sure if we should bake in a polyfill or let the user choose one though, especially since I don't know the license of that code.

@hdgarrood
Copy link
Contributor

I think it would be best to include this polyfill; if a better polyfill does exist we can always update this later. Not dealing with this here will result in a poor UX for devs who need to support IE, I think. The code is public domain; see https://developer.mozilla.org/en-US/docs/MDN/About#Copyrights_and_licenses:

Code samples added on or after August 20, 2010 are in the public domain. No licensing notice is necessary, but if you need one, you can use: "Any copyright is dedicated to the Public Domain. http://creativecommons.org/publicdomain/zero/1.0/".

The Math.imul page was created in 2012.

@paf31
Copy link
Contributor

paf31 commented Nov 20, 2017

Okay, thanks Harry. Sounds good to me then, assuming we can check that DCE works, or modify if necessary.

@matthewleon
Copy link
Contributor Author

Okay, I've convinced jshint that I'm not a war criminal. I compiled with -Oand the output still includes the polyfill. Is there a further check that should be performed for DCE? Including this in a project with a Main and bundling?

@paf31
Copy link
Contributor

paf31 commented Nov 20, 2017

Yes, you would need to reference it from some separate entry point, I think.

@matthewleon
Copy link
Contributor Author

I've created a branch with a Main: https://github.com/matthewleon/purescript-math/tree/imul-test-dce

main = logShow $ 3 `imul` 5

I've pasted the output of pulp browserify -O below. DCE preserves the polyfill.

(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){
// Generated by purs bundle 0.11.7
var PS = {};
(function(exports) {
    "use strict";

  exports.log = function (s) {
    return function () {
      console.log(s);
      return {};
    };
  };
})(PS["Control.Monad.Eff.Console"] = PS["Control.Monad.Eff.Console"] || {});
(function(exports) {
    "use strict";

  exports.showIntImpl = function (n) {
    return n.toString();
  };
})(PS["Data.Show"] = PS["Data.Show"] || {});
(function(exports) {
  // Generated by purs version 0.11.7
  "use strict";
  var $foreign = PS["Data.Show"];     
  var Show = function (show) {
      this.show = show;
  };                                                 
  var showInt = new Show($foreign.showIntImpl);
  var show = function (dict) {
      return dict.show;
  };
  exports["Show"] = Show;
  exports["show"] = show;
  exports["showInt"] = showInt;
})(PS["Data.Show"] = PS["Data.Show"] || {});
(function(exports) {
  // Generated by purs version 0.11.7
  "use strict";
  var $foreign = PS["Control.Monad.Eff.Console"];
  var Control_Monad_Eff = PS["Control.Monad.Eff"];
  var Data_Show = PS["Data.Show"];
  var Data_Unit = PS["Data.Unit"];
  var logShow = function (dictShow) {
      return function (a) {
          return $foreign.log(Data_Show.show(dictShow)(a));
      };
  };
  exports["logShow"] = logShow;
})(PS["Control.Monad.Eff.Console"] = PS["Control.Monad.Eff.Console"] || {});
(function(exports) {
    "use strict";            

  function nativeImul(a) {
    return function (b) {
      return Math.imul(a, b);
    };
  }

  // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/imul
  function emulatedImul(a) {
    /*jshint bitwise: false*/
    return function (b) {
      var ah = a >>> 16 & 0xffff;
      var al = a & 0xffff;
      var bh = b >>> 16 & 0xffff;
      var bl = b & 0xffff;
      // the shift by 0 fixes the sign on the high part
      // the final |0 converts the unsigned value into a signed value
      return al * bl + (ah * bl + al * bh << 16 >>> 0) | 0;
    };
  }

  exports.imul = Math.imul ? nativeImul : emulatedImul;
})(PS["Math"] = PS["Math"] || {});
(function(exports) {
  // Generated by purs version 0.11.7
  "use strict";
  var $foreign = PS["Math"];
  exports["imul"] = $foreign.imul;
})(PS["Math"] = PS["Math"] || {});
(function(exports) {
  // Generated by purs version 0.11.7
  "use strict";
  var Control_Monad_Eff = PS["Control.Monad.Eff"];
  var Control_Monad_Eff_Console = PS["Control.Monad.Eff.Console"];
  var Data_Function = PS["Data.Function"];
  var Data_Show = PS["Data.Show"];
  var $$Math = PS["Math"];
  var Prelude = PS["Prelude"];        
  var main = Control_Monad_Eff_Console.logShow(Data_Show.showInt)($$Math.imul(3)(5));
  exports["main"] = main;
})(PS["Main"] = PS["Main"] || {});
PS["Main"].main();

},{}]},{},[1]);

@paf31
Copy link
Contributor

paf31 commented Nov 22, 2017

I'm fine with this then. Anyone else have comments?

@hdgarrood
Copy link
Contributor

Perhaps it should say in the docs that the imul that is exported from the Math module will use the native Math.imul if it is available or a polyfill otherwise? Looks great though 👍

@minoki minoki mentioned this pull request Mar 3, 2018
@hdgarrood hdgarrood merged commit a41ed69 into purescript-deprecated:master Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants