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

Get rid of static Math class #1356

Closed
munificent opened this issue Jan 26, 2012 · 11 comments
Closed

Get rid of static Math class #1356

munificent opened this issue Jan 26, 2012 · 11 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. type-enhancement A request for a change that isn't a bug

Comments

@munificent
Copy link
Member

When Math was first designed, Dart didn't have top-level functions. Things like abs() are methods of Math because they had to be methods of some class at the time.

Now we have top-level functions, libraries and prefixes. Given that, it seems natural to me to use those for the math functions. That way if users don't want prefixes for abs(), etc. they don't have to. If they do, they can always import it with a prefix.

@DartBot
Copy link

DartBot commented Jan 27, 2012

This comment was originally written by @seaneagan


I think I remember this being discussed before, but this could be done with a "dart:math" library which is implicitly imported with a default prefix just like "dart:core" unless you explicitly import it. The only question is whether the default prefix should be "" or "math". I would say "math", in order to make it safer to add to / update "dart:math" in the future.

@munificent
Copy link
Member Author

That would work. Personally, I'd like to change how Dart handles name collisions such that you only have to worry about prefixes when a collision actually occurs. That should ease the forward compatibility problems of unprefixed imports.

@lrhn
Copy link
Member

lrhn commented Jun 22, 2012

We could make a dart:math library that is imported by default (as part of the core library) with the prefix "Math". If anybody wants to import it with a different prefix (including none), they can do that too.

@DartBot
Copy link

DartBot commented Jun 30, 2012

This comment was originally written by @seaneagan


I think the naming convention for prefixes should be lowerCamelCase (i.e. "math") to distinguish a prefix namespace from a class' static namespace.

Also, I think it was recently decided to disallow overriding the implicit unprefixed dart:core import with a prefixed one (though you can add a prefixed dart:core import), so any implicit dart:math import should be consistent with that. However, I'd be perfectly fine with having to explicitly import dart:math, especially if the import syntax is made less verbose.

@lrhn
Copy link
Member

lrhn commented Jul 2, 2012

We should make a dart:math library across all implementations. The VM already has it (it includes the static methods currently on Math and a separate Random class).
It is currently imported with prefix Math for backwards compatibility, and we should keep doing that until we can make a coordinated pre-announced change to remove it.


Added Accepted label.

@lrhn
Copy link
Member

lrhn commented Jul 5, 2012

Dart2js also has a dart:math library. Like the VM, it also still have a static Math class with the same content (not an import of the library with prefix "Math" - as seen by the fact that you can extend the Math class).

I think we should just remove Math completely, and make importing dart:math optional, when we revisit the core library. Most code won't need the math functions anyway.

@munificent
Copy link
Member Author

I think we should just remove Math completely, and make importing dart:math optional, when we revisit the core library.

+1

Most code won't need the math functions anyway.

Agreed, except for min() and max() which I find useful in most general-purpose code, and parseInt() and parseDouble(). Strawman:

I find it confusing anyway that abs() and min() and max() aren't located together, so maybe move min() and max() to be instance methods?

    var a = 123;
    var c = a.max(234);
    assert(c == 234);

Likewise, parseInt() and parseDouble() could be factory constructors on int and num:

    var n = new int.parse("123");
    var d = new double.parse("12.3");

I'd also add parse() to num, which will return a double or an int as appropriate.

@DartBot
Copy link

DartBot commented Jul 9, 2012

This comment was originally written by @seaneagan


maybe move min() and max() to be instance methods?

+1, I would define them as instance methods on Comparable<T> as suggested in:

http://code.google.com/p/dart/issues/detail?id=1492#c3

... it would be a small copy/paste pain to have to implement these for all Comparables prior to mixins being added to the language, but I think it would be worth it.

Likewise, parseInt() and parseDouble() could be factory constructors on int and num:

see issue #2868

I'd also add parse() to num, which will return a double or an int as appropriate.

see issue #1981

@DartBot
Copy link

DartBot commented Jul 11, 2012

This comment was originally written by @seaneagan


regarding min and max as instance methods see issue #3176

@sethladd
Copy link
Contributor

I think we can close this. The new dart:math library is the new hotness here. It's full of top level methods.

@madsager
Copy link
Contributor

Added Fixed label.

@munificent munificent added Type-Enhancement area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Aug 22, 2012
@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed type-enhancement labels Mar 1, 2016
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants