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

Add c++ ABI tests #4952

Merged
merged 1 commit into from
Sep 14, 2015
Merged

Add c++ ABI tests #4952

merged 1 commit into from
Sep 14, 2015

Conversation

gchatelet
Copy link
Contributor

@yebblies Here is a first simple version of C++ abi tests.
It fails for real numbers on Linux.

@gchatelet
Copy link
Contributor Author

So real type compiled with -O seems to produce an invalid value when passing the value 0
See failing test.

@dnadlinger
Copy link
Member

Your comparison includes the padding bytes, which are not guaranteed to be set to any particular value. (At least not by C/C++.)

@gchatelet
Copy link
Contributor Author

@klickverbot I see, so for real values I should check only for the first 80 bits ?

@dnadlinger
Copy link
Member

real is not always 80 bit once you leave DMD-land, but yeah, that's the idea. @ibuclaw can probably tell you exactly how he'd like to see this implemented. Can't you just use is, though?

@gchatelet
Copy link
Contributor Author

is or == doesn't work with NaN since NaN==NaN is always false.
I think a better strategy is to use == expect when value is NaN.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 24, 2015

is will use memcmp at the correct precision for you (so you don't need to have logic in your own tests for that).

@dnadlinger
Copy link
Member

@gchatelet: I'm pretty sure nan is nan.

@jmdavis
Copy link
Member

jmdavis commented Aug 25, 2015

@gchatelet: I'm pretty sure nan is nan.

Except that there are different variants of NaN, so two NaNs may not be the same via is. e.g. https://github.com/D-Programming-Language/phobos/blob/master/std/bitmanip.d#L2288

@yebblies
Copy link
Member

Checking float equality with is is the correct thing to do, but be wary of https://issues.dlang.org/show_bug.cgi?id=3632

We really do want to be checking that the floats match bit for bit.

@gchatelet
Copy link
Contributor Author

So as @jmdavis pointed out two NaNs might not have the same representation. Especially since the value is copied in C++ passthrough functions. The C++ code might choose a different representation for NaN.

Options are:

  1. For robustness and also valid semantic I'll use std.math.isIdentical for floating point and is otherwise.
  2. Do not use value NaN or 0 for floating points since they can have different binary representations.
  3. Special case floating points on T.mant_dig as in std.math

I went for 1. right now. What do you think ?

@gchatelet
Copy link
Contributor Author

BTW the Windows tests are failing, I suspect a name mangling issue.

@yebblies
Copy link
Member

Phobos should not be used in the dmd test suite.

@gchatelet
Copy link
Contributor Author

Please take another look. There is now no more dependencies on phobos.
Parameters are now passed: by value, by pointer and by reference.

@yebblies
Copy link
Member

Looks good. Please squash the commits together.

@gchatelet
Copy link
Contributor Author

Done. Thx for the review.

@dnadlinger
Copy link
Member

Auto-merge toggled on

@dnadlinger
Copy link
Member

The funny pointer type formatting is not my cup of tea, but LGTM.

dnadlinger added a commit that referenced this pull request Sep 14, 2015
@dnadlinger dnadlinger merged commit 8e90887 into dlang:master Sep 14, 2015
@gchatelet
Copy link
Contributor Author

thx @klickverbot . What do you mean by funny pointer ? Having the star next to the type instead of next to the variable ? I don't mind providing a fix if you want.

@dnadlinger
Copy link
Member

It's not worth the effort, and Walter has been doing this from time to time too, but I'm really not a fan of having the start anywhere but next to the type in D.

@yebblies
Copy link
Member

Having the star next to the type instead of next to the variable ?

Yes, that's correct C++ style but incorrect D style, since the association is different.

@dnadlinger
Copy link
Member

This seems to have triggered a spurious failure in the "stage 3" GDC DDMD tests on Travis: https://travis-ci.org/D-Programming-Language/dmd/builds/80190427 ?!

@gchatelet
Copy link
Contributor Author

Hmm why do you think this is linked to this PR, it looks like it's linked to druntime, not dmd tests.

make[2]: Entering directory `/home/travis/build/D-Programming-Language/druntime/test/profile'
Testing profile
make[2]: execvp: ./obj/linux/64/profile: Permission denied
make[2]: *** [obj/linux/64/profile.done] Error 127

@dnadlinger
Copy link
Member

It seems improbable that the failure would be related to this. I've invalidated the test run; let's see whether it comes up again.

@gchatelet gchatelet deleted the add_abi_tests branch September 17, 2015 19:43
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.

5 participants