-
Notifications
You must be signed in to change notification settings - Fork 38
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 32-bit build #745
Add 32-bit build #745
Conversation
0eedfa1
to
a5b8a06
Compare
3f0abae
to
f9c35c6
Compare
@chfast Can we get this merged by disabling FP tests on 32-bit, for now? |
Yes. |
3f8ed8c
to
46764cd
Compare
Codecov Report
@@ Coverage Diff @@
## master #745 +/- ##
==========================================
- Coverage 99.02% 99.02% -0.01%
==========================================
Files 80 80
Lines 12789 12788 -1
==========================================
- Hits 12664 12663 -1
Misses 125 125
Flags with carried forward coverage won't be shown. Click here to find out more.
|
build_type: Debug | ||
cmake_options: -DCMAKE_TOOLCHAIN_FILE=~/project/cmake/toolchains/32bit.cmake | ||
- test | ||
# TODO: Disabled, because fixes to NaN payload handling in the tool are needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you are 100% sure it is only a tooling fix (and no fix/change needed in the interpreter), I'm fine merging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but this is the first thing to fix.
cbc91ac
to
75d5abf
Compare
@@ -1360,6 +1372,10 @@ TEST(execute_floating_point, f64_add_off_by_one) | |||
_FPU_SETCW(fpu_cw); | |||
*/ | |||
|
|||
#if defined(__i386__) && !defined(__SSE_MATH__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not at the top of the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To execute as much as possible. Can be at the top too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On top it would be more clear that long comment there explains the reason for skipping IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Will do it.
Depends on #744.