Skip to content

Conversation

@jwagenleitner
Copy link
Contributor

@jwagenleitner jwagenleitner commented Sep 2, 2016

Just thought I'd take a stab at implementing this, maybe there are performance concerns with this. It may be more efficient to use FLOAD/DLOAD before the isNaN call instead of the DUP2 up front, but in the context of those methods I wasn't sure how to access the index of the variable on the stack.

@paulk-asert
Copy link
Contributor

+1, looks okay to me but I'm not super familiar with how performance might be affected at this level

@jwagenleitner
Copy link
Contributor Author

Thanks for looking it over, I appreciate it.

I'll let it sit for a while and see if I can get some time to do a quick benchmark. Not sure how often floats/doubles are used in tight loops and compared using the Groovy Truth, but probably wouldn't hurt to make sure it's not too costly of an extra check for NaN.

@jwagenleitner jwagenleitner force-pushed the GROOVY-4018-groovytruth-NaN branch from f5f090a to ceb9e18 Compare July 1, 2017 18:35
@jwagenleitner jwagenleitner force-pushed the GROOVY-4018-groovytruth-NaN branch 3 times, most recently from 3a22891 to d7f5867 Compare July 8, 2017 22:22
@jwagenleitner jwagenleitner force-pushed the GROOVY-4018-groovytruth-NaN branch from d7f5867 to bd416bb Compare July 8, 2017 22:55
@jwagenleitner
Copy link
Contributor Author

Updated the PR to consolidate the primitive-to-boolean conversion for both prim opts and static compilation and changed since tags to 3.0.0.

@paulk-asert
Copy link
Contributor

LGTM. How about we change the since version to 2.6.0 and merge into master and GROOVY_2_6_X?

@asfgit asfgit closed this in 344c884 Oct 2, 2017
@jwagenleitner jwagenleitner deleted the GROOVY-4018-groovytruth-NaN branch October 2, 2017 00:48
@jwagenleitner
Copy link
Contributor Author

Thanks Paul, I updated the since tags and merged into the branches suggested.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants