Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

frog should strip out calls to toDouble #773

Closed
DartBot opened this issue Dec 9, 2011 · 6 comments
Closed

frog should strip out calls to toDouble #773

DartBot opened this issue Dec 9, 2011 · 6 comments
Labels
closed-not-planned Closed as we don't intend to take action on the reported issue P3 A lower priority bug or feature request

Comments

@DartBot
Copy link

DartBot commented Dec 9, 2011

This issue was originally filed by [email protected]


In a program like this:

  int x = 23;
  double n = x.toDouble();

frog inserts an actual call to a function called toDouble (which is a no-op) in the generated code. It seems like this should get stripped out. (Or, another solution would be to not require the programmer to put it there in the first place, but I think that would be a language change.)

I'm not sure whether to regard this as a language issue or a frog issue, but somehow I think we need to this assignment without involving a real function call in the generated js.

@rakudrama
Copy link
Member

You can only do what you suggest if you prove:
(1) x is not null on the second line
(2) the representation of the value of x is already double.

I consider (1) to be part language issue. Having nullable and non-nullable types would help with the 'proof'.

(2) depends a lot on whether we can continue to use JS numbers for all values of type int.

The generated code is quite odious since it causes the boxing of the JS number, only to forcefully unbox it. Both the boxing and unboxing (via this + 0) cause V8 to generate slow-path code. That could be ameliorated by generating a special dispatch.

@floitschG
Copy link
Contributor

V8 doesn't need to box the number if "use strict" is used.
I opened a bug for that: Issue #781.

@DartBot
Copy link
Author

DartBot commented Dec 9, 2011

This comment was originally written by [email protected]


Added Triaged label.

@DartBot
Copy link
Author

DartBot commented Dec 9, 2011

This comment was originally written by [email protected]


I think the null handling is probably a separate issue. (In this example it would be fine if 'n' is assigned null if 'x' was null.)
The situation is I think the common case where you want to assign an int to a variable of type double. Currently we require the programmer to explicitly write the toDouble() call. This is OK (perhaps verbose, but arguably it has the advantage that the programmer sees that a conversion is involved that might lose precision). What I think is a problem is that the generated js is more roundabout than the equivalent hand-written javascript that would simply say: n = x.

@jmesserly
Copy link

as Stephen said, we need to call a method so we get a null check, or else insert the null check some other way.

@jmesserly
Copy link

Also, I think there's a readability argument to be made for keeping the JS code similar to the Dart code. Unless this is motivated by performance, I don't see the argument for changing it, other than switching to "use strict" as Florian suggested.


Removed Priority-Medium label.
Added Priority-Low, WontFix labels.

@DartBot DartBot added Type-Defect P3 A lower priority bug or feature request labels Dec 20, 2011
@kevmoo kevmoo added closed-not-planned Closed as we don't intend to take action on the reported issue and removed resolution-wont_fix labels Mar 1, 2016
copybara-service bot pushed a commit that referenced this issue Nov 28, 2022
Revisions updated by `dart tools/rev_sdk_deps.dart`.

ffi (https://github.com/dart-lang/ffi/compare/3ede231..17a8142):
  17a8142  2022-11-21  Daco Harkes  Add links to API reference and examples to README (#167)

fixnum (https://github.com/dart-lang/fixnum/compare/bca3816..62916f2):
  62916f2  2022-11-24  Lasse R.H. Nielsen  Split into separate libraries instead of using parts. (#97)
  14d4827  2022-11-23  Lasse R.H. Nielsen  Add `tryParse` methods. (#96)

http (https://github.com/dart-lang/http/compare/d56141d..047d6ed):
  047d6ed  2022-11-22  Brian Quinlan  Fixes a bug where requests made in different Clients would fail (#827)
  9dbbafb  2022-11-17  Brian Quinlan  Add tests for compressed responses (#823)

markdown (https://github.com/dart-lang/markdown/compare/37951d1..ee3f4e9):
  ee3f4e9  2022-11-24  Zhiguang Chen  Fix a blockquote issue (#496)
  c9048c0  2022-11-16  Zhiguang Chen  Optimise DelimiterSyntax (#492)

protobuf (https://github.com/dart-lang/protobuf/compare/ae90e53..c181573):
  c181573  2022-11-23  Kevin Moore  [api_benchmark] migrate to null-safety (#776)
  648deaf  2022-11-23  Kevin Moore  Standardize and fix lints (#775)
  bfa4c0d  2022-11-23  Kevin Moore  Fix `///` at the top of generated Dart files (#774)
  ed68426  2022-11-17  Devon Carew  Remove duplicate consts (#773)

sse (https://github.com/dart-lang/sse/compare/283568d..8d018dd):
  8d018dd  2022-11-23  Elliott Brooks (she/her)  Format markdown files (#68)
  2f6f151  2022-11-23  Elliott Brooks (she/her)  Add optional `debugKey` parameter to SSE client (#67)
  eaee6a8  2022-11-23  Elliott Brooks (she/her)  Use Fetch API in SSE Client (#66)

test (https://github.com/dart-lang/test/compare/7756833..b25dac9):
  b25dac99  2022-11-21  Kevin Moore  checks: finish async tests (#1793)
  3166163e  2022-11-18  Devon Carew  Update scorecards-analysis.yml (#1792)
  d930d5b0  2022-11-18  Nate Bosch  Add support for async soft checks (#1789)
  b1411a21  2022-11-18  Devon Carew  blast_repo fixes (#1788)
  a6aa04e0  2022-11-18  Jacob MacDonald  disable wasm tests for now as they are failing (#1791)
  8b5174c1  2022-11-17  Kevin Moore  Starting on async tests (#1787)

Change-Id: Ic361fce7753d08d43fc827f13cb1bfa1738cc16e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/272343
Auto-Submit: Devon Carew <[email protected]>
Reviewed-by: Kevin Moore <[email protected]>
Commit-Queue: Devon Carew <[email protected]>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-not-planned Closed as we don't intend to take action on the reported issue P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

5 participants