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

add more parseFloat parseInt failing tests #4132

Closed
wants to merge 2 commits into
base: development
from

Conversation

Projects
None yet
5 participants
@exFalso

exFalso commented Apr 3, 2015

No description provided.

@nadako

This comment has been minimized.

Show comment
Hide comment
@nadako

nadako Apr 4, 2015

Member

i don't think we should support scientific notation for parseInt. not sure about spaces.

Member

nadako commented Apr 4, 2015

i don't think we should support scientific notation for parseInt. not sure about spaces.

@exFalso

This comment has been minimized.

Show comment
Hide comment
@exFalso

exFalso Apr 4, 2015

@nadako These are examples where js/java/cs behaviour differs. Because these are mostly invalid input strings I think any behaviour is fine as long as it's consistent across backends.
My reasoning for the expected values was that parseInt("0x C") should not be a well-formed hexa, so it should be treated as a "0" with some garbage at the end. Similarly for the other cases

exFalso commented Apr 4, 2015

@nadako These are examples where js/java/cs behaviour differs. Because these are mostly invalid input strings I think any behaviour is fine as long as it's consistent across backends.
My reasoning for the expected values was that parseInt("0x C") should not be a well-formed hexa, so it should be treated as a "0" with some garbage at the end. Similarly for the other cases

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Apr 4, 2015

Member

I think the only way to enforce consistent behavior even for invalid strings would be to make a pure-Haxe parsing implementation and use that for all targets. We could keep Std.parseFloat the way it is and document it as using the native parsing implementation if available. A consistent parser could then be added as Float.parse like Java does it.

Member

Simn commented Apr 4, 2015

I think the only way to enforce consistent behavior even for invalid strings would be to make a pure-Haxe parsing implementation and use that for all targets. We could keep Std.parseFloat the way it is and document it as using the native parsing implementation if available. A consistent parser could then be added as Float.parse like Java does it.

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Apr 4, 2015

Member

We should do as we do with other parts of std lib : specify what SHOULD work and leave unspecified all the other invalid values results.

Member

ncannasse commented Apr 4, 2015

We should do as we do with other parts of std lib : specify what SHOULD work and leave unspecified all the other invalid values results.

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Apr 4, 2015

Member

Yeah but why not have a parallel pure-Haxe implementation that works the same everywhere? That way you can trade some performance for consistency if you choose to do so.

Member

Simn commented Apr 4, 2015

Yeah but why not have a parallel pure-Haxe implementation that works the same everywhere? That way you can trade some performance for consistency if you choose to do so.

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Apr 4, 2015

Member

@Simn I don't think consistency over invalid cases is something we should encourage, and you don't agree with it as well in the case of Std.is for instance :)

Member

ncannasse commented Apr 4, 2015

@Simn I don't think consistency over invalid cases is something we should encourage, and you don't agree with it as well in the case of Std.is for instance :)

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Apr 4, 2015

Member

That's different, my Problem with Std.is is that it admits invalid cases which wouldn't even make it past the parsing stage in any other language. That's just bad design on our end so I don't care about it because we'll have a proper is operator at some point.

Here we have a clear String -> Float signature and I don't see why it would hurt to have a consistent implementation for that. In fact, I'm really surprised you're even arguing against it.

Member

Simn commented Apr 4, 2015

That's different, my Problem with Std.is is that it admits invalid cases which wouldn't even make it past the parsing stage in any other language. That's just bad design on our end so I don't care about it because we'll have a proper is operator at some point.

Here we have a clear String -> Float signature and I don't see why it would hurt to have a consistent implementation for that. In fact, I'm really surprised you're even arguing against it.

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Apr 4, 2015

Member

@Simn a lot of places in the std library are meant to work "as intended", which mean that we don't cover the invalid usages. Everything involving Dynamic or String parsing is usually the case (Reflection, Date fromString, etc) and I think it's better to keep it this way in order to prevent code bloating by having to handle all the invalid cases in a consistent manner.

Member

ncannasse commented Apr 4, 2015

@Simn a lot of places in the std library are meant to work "as intended", which mean that we don't cover the invalid usages. Everything involving Dynamic or String parsing is usually the case (Reflection, Date fromString, etc) and I think it's better to keep it this way in order to prevent code bloating by having to handle all the invalid cases in a consistent manner.

@nadako

This comment has been minimized.

Show comment
Hide comment
@nadako

nadako Apr 4, 2015

Member

I agree with nicolas.

Member

nadako commented Apr 4, 2015

I agree with nicolas.

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Apr 4, 2015

Member

I really don't understand how you can be okay with one target parsing an invalid value and another one throwing an exception. That's exactly the kind of crap that annoys people who switch their code to a different target because suddenly that cross-platform toolkit is not cross-platform anymore. And that for the doubtful benefit of having one less function in the standard library which according to you is "batteries included".

Is it really so hard to believe that some people might need a parsing function that works consistently even for invalid input?

Member

Simn commented Apr 4, 2015

I really don't understand how you can be okay with one target parsing an invalid value and another one throwing an exception. That's exactly the kind of crap that annoys people who switch their code to a different target because suddenly that cross-platform toolkit is not cross-platform anymore. And that for the doubtful benefit of having one less function in the standard library which according to you is "batteries included".

Is it really so hard to believe that some people might need a parsing function that works consistently even for invalid input?

@exFalso

This comment has been minimized.

Show comment
Hide comment
@exFalso

exFalso Apr 4, 2015

This coming from a "customer" point of view:)

We had quite a few problems because of inconsistencies between js and java when we changed the backend of the haxe code in one of our projects. We would gladly trade off performance in order to gain consistency. Another good example is Array.sort vs haxe.ds.ArraySort.sort. (Oh this reminded be of a bug in the java backend with multiple catch blocks, will submit an issue)

I think even if you decide to go for native performance over consistency, you should clearly define the preconditions under which the behaviour is consistent. For example in the case of parseFloat you could give the BNF of valid inputs.
This way at least the users have the chance to provide the boilerplate code around std function calls to handle invalid cases. Right now our "solution" is to have our own Std.hx that overrides the actual std lib.

exFalso commented Apr 4, 2015

This coming from a "customer" point of view:)

We had quite a few problems because of inconsistencies between js and java when we changed the backend of the haxe code in one of our projects. We would gladly trade off performance in order to gain consistency. Another good example is Array.sort vs haxe.ds.ArraySort.sort. (Oh this reminded be of a bug in the java backend with multiple catch blocks, will submit an issue)

I think even if you decide to go for native performance over consistency, you should clearly define the preconditions under which the behaviour is consistent. For example in the case of parseFloat you could give the BNF of valid inputs.
This way at least the users have the chance to provide the boilerplate code around std function calls to handle invalid cases. Right now our "solution" is to have our own Std.hx that overrides the actual std lib.

@nadako

This comment has been minimized.

Show comment
Hide comment
@nadako

nadako Apr 4, 2015

Member

Is it really so hard to believe that some people might need a parsing function that works consistently even for invalid input?

Then we should change the general idea of std lib to provide "always consistent, but slow" functions instead of how we do now.

Member

nadako commented Apr 4, 2015

Is it really so hard to believe that some people might need a parsing function that works consistently even for invalid input?

Then we should change the general idea of std lib to provide "always consistent, but slow" functions instead of how we do now.

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Apr 4, 2015

Member

The promise of Haxe is to keep native speed as much as possible, and if we managed to do that it's because we are not overspecifying the way things works. If people want 100% consistency, they should use a VM that guarantee such thing.

In some cases the inconsistencies becomes so much hard to deal with for the end user that we indeed switched to a pure haxe implementation (in the case of Xml parsing for instance). But in the general case, we should clearly specify what is supposed to work (I'm fine including all accepted formats regexps in parseInt/parseFloat documentation), and leave the other cases unspecified.

We could however give some guarantees such as "unspecified formats might return an invalid value or 0/NaN, but will not throw an exception"

Member

ncannasse commented Apr 4, 2015

The promise of Haxe is to keep native speed as much as possible, and if we managed to do that it's because we are not overspecifying the way things works. If people want 100% consistency, they should use a VM that guarantee such thing.

In some cases the inconsistencies becomes so much hard to deal with for the end user that we indeed switched to a pure haxe implementation (in the case of Xml parsing for instance). But in the general case, we should clearly specify what is supposed to work (I'm fine including all accepted formats regexps in parseInt/parseFloat documentation), and leave the other cases unspecified.

We could however give some guarantees such as "unspecified formats might return an invalid value or 0/NaN, but will not throw an exception"

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Apr 4, 2015

Member

That would be the worst of both worlds: Make some default implementations slower but still don't provide a truly consistent cross-target behavior.

Specification is nice but it's not a solution. These inconsistencies put strain on developers because they have to remember them, even if it's just for rare cases. Seeing that a partner like prezi has to use their own implementation of Std.hx (this is how forks start btw) worries me and seeing that you're blocking this off for no good reason (I'm seeing) worries me even more.

Member

Simn commented Apr 4, 2015

That would be the worst of both worlds: Make some default implementations slower but still don't provide a truly consistent cross-target behavior.

Specification is nice but it's not a solution. These inconsistencies put strain on developers because they have to remember them, even if it's just for rare cases. Seeing that a partner like prezi has to use their own implementation of Std.hx (this is how forks start btw) worries me and seeing that you're blocking this off for no good reason (I'm seeing) worries me even more.

@waneck

This comment has been minimized.

Show comment
Hide comment
@waneck

waneck Apr 29, 2015

Is this really supposed to work? @ncannasse

waneck commented on tests/unit/src/unitstd/Std.unit.hx in 6993078 Apr 29, 2015

Is this really supposed to work? @ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Apr 29, 2015

As I said on the thread, I don't think we should specify how invalid inputs are parsed, feel free to comment out these

ncannasse replied Apr 29, 2015

As I said on the thread, I don't think we should specify how invalid inputs are parsed, feel free to comment out these

waneck added a commit that referenced this pull request Apr 29, 2015

waneck added a commit to waneck/haxe that referenced this pull request Apr 29, 2015

waneck added a commit that referenced this pull request Apr 29, 2015

waneck added a commit that referenced this pull request Apr 29, 2015

waneck added a commit that referenced this pull request May 4, 2015

waneck added a commit that referenced this pull request May 4, 2015

waneck added a commit that referenced this pull request May 4, 2015

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Nov 29, 2015

Member

Another discussion I'm coming back to and I still don't see how anyone in his right mind would be against adding a pure-Haxe Float.parse(s:String) implementation that users can choose to use if they so desire. What am I missing here?

Member

Simn commented Nov 29, 2015

Another discussion I'm coming back to and I still don't see how anyone in his right mind would be against adding a pure-Haxe Float.parse(s:String) implementation that users can choose to use if they so desire. What am I missing here?

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Nov 29, 2015

Member

Several comes to my mind:

  • having several implementations for the same things, with different developers in the same team using one or the other, leading to stubble bugs even on the same platform
  • an increase in codesize, for people using -dce no (usage of reflection, JS shared code, etc)
  • a slower version which will make people hastly deduce that Haxe is slower than X
  • Float being sometimes reserved identifier, more runtime hacks required to store it in FloatImpl + thus to reflection possible (in hscript for instance)

In general I'm more in favor of having a good spec + using native implementation, with a few tweaks if necessary. than having a reference implementation that performs slow everywhere and people can't rely on when they want to do code that works in decent time on mobile for instance.

Member

ncannasse commented Nov 29, 2015

Several comes to my mind:

  • having several implementations for the same things, with different developers in the same team using one or the other, leading to stubble bugs even on the same platform
  • an increase in codesize, for people using -dce no (usage of reflection, JS shared code, etc)
  • a slower version which will make people hastly deduce that Haxe is slower than X
  • Float being sometimes reserved identifier, more runtime hacks required to store it in FloatImpl + thus to reflection possible (in hscript for instance)

In general I'm more in favor of having a good spec + using native implementation, with a few tweaks if necessary. than having a reference implementation that performs slow everywhere and people can't rely on when they want to do code that works in decent time on mobile for instance.

@nadako

This comment has been minimized.

Show comment
Hide comment
@nadako

nadako Mar 10, 2016

Member

So, are going to proceed somehow here?

Member

nadako commented Mar 10, 2016

So, are going to proceed somehow here?

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Mar 11, 2016

Member

I agree that we need to specify the valid BNF in parseInt/parseFloat. Everything else is unspecified and should be kept this way.

Member

ncannasse commented Mar 11, 2016

I agree that we need to specify the valid BNF in parseInt/parseFloat. Everything else is unspecified and should be kept this way.

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Mar 20, 2017

Member

Since the agreement seems to be that invalid cases are unspecified, there's no need to merge this PR.

Member

Simn commented Mar 20, 2017

Since the agreement seems to be that invalid cases are unspecified, there's no need to merge this PR.

@Simn Simn closed this Mar 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment