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 IEEE754 files to Visual Studio #948

Merged
merged 7 commits into from
May 2, 2016
Merged

Conversation

Andne
Copy link
Contributor

@Andne Andne commented Apr 26, 2016

Also adds a check for _MSC_VER to disable the FENV logic for versions prior to 2013. Older versions don't have the fenv.h header file that is required for this to work properly.

This should fix the issue seen in #946.

@@ -71,3 +75,5 @@ void set_everything_c()
set_invalid_c();
set_inexact_c();
}

#endif // CPPUTEST_HAVE_FENV
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove the comment. That will make the compiler happy, and @basvodde, too ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems Wcl objects to the missing newline at the end of file, not sure though.

@arstrube
Copy link
Contributor

Thanks for taking care of this :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.909% when pulling 9500d53 on Andne:vs2005 into fc9317d on cpputest:master.

@Andne
Copy link
Contributor Author

Andne commented Apr 27, 2016

@arstrube The build looks good now, but for some reason VS2015 (v140) has a couple failures. Any thoughts on this? Should we try to fix them or just disable the plugin completely on Visual C++ for now?

I've also noticed that since we run the tests as part of the build step normally, it's causing the failures to show as build failures instead of failed tests. Should maybe look at what it would take to cleanup this behavior as well. Not really specific to this problem, just a general build cleanup issue.

@@ -25,6 +25,10 @@
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

#include <CppUTest/CppUTestConfig.h>

#ifdef CPPUTEST_HAVE_FENV
Copy link
Contributor

@arstrube arstrube Apr 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this? There is nothing in this file that depends on whether or not fenv.h is present, or IEEE754 checks are supported. That is the whole point. This is "production code like". It should be ignorant IEEE754 stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because of the INFINITE define later on. For at least some versions of Visual C++, INIFINITE isn't defined and so I was getting compiler errors. This was the quickest way I thought of to avoid that issue (the file doesn't get used unless FENV is active), but the correct solution probably would have been to just block off the one function if INFINITE is missing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably shouldn't be using INFINITY in this file at all. Any idea how to rewrite the fucntion without INFINITY?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking online a bit it appears that there's an isfinite(float) function that could maybe be used instead. I don't have access to the older compilers right now to check though. Some of the information that I found implies that you're supposed to use isfinite() instead of comparing against INFINITY anyways.

Judging from online documentation, the closest in older VC++ implementations is going to be _finite(double). Looks like that provides the same functionality just under a different name. It seems a bit ugly to wrap it in a macro just for Visual C, but I don't really know another solution and it's probably cleaner than excluding the entire file. However, the functions in the file are unused unless FENV is enabled anyways, so how much does it actually matter if the file is compiled when FENV isn't active?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it doesn't :-). Actually I was hoping you might know some clever way to run into infinity without having to use a #define at all.... like FLOAT_MAX * 2 or something (I'm not really serious, because that would just be a different define ;-). Because it would be nice if this file contained nothing but plain C code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void set_overflow_c(void)
{
    f = 1e38f;
    f *= f;
}

appears to work, at least on my machine. If it survives Travis, then I guess I could use that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind putting this in your PR (or I can submit a separate one):

void set_overflow_c(void)
{
    f = 1e38f;
    f *= f;
}

void set_underflow_c(void)
{
    f = 1e-38f;
    f *= f;
}

I couldn't find any good substitute for sqrt(-1), so math.h will have to stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added that and removed the #ifdef from the file.

@arstrube
Copy link
Contributor

Hi Andy, this looks indeed good (except for the one point noted above). The failures are due to the fact that compilers don't seem to implement some of the checks consistently. FE_INVALID is one of those. It doesn't mean that it's not working. We have several options here:

  1. Remove FE_INVALID from all tests.
  2. Use #ifdef in some way to exempt MS from these tests.
  3. Set CPPUTEST_FENV_IS_WORKING_PROPERLY to 0 for MS (no tests will be run).

@basvodde what do you think?

Don't need to block compilation now, should work with any compiler
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.909% when pulling bf58b42 on Andne:vs2005 into fc9317d on cpputest:master.

@Andne
Copy link
Contributor Author

Andne commented Apr 30, 2016

I think I have IEEE754PluginTest_c.c cleaned up, any thoughts on how to handle VS2013 and VS2015? They can pass some of the tests but a couple of them still fail.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.909% when pulling 7d99131 on Andne:vs2005 into fc9317d on cpputest:master.

@arstrube
Copy link
Contributor

I think both failures are due to the invalid flag. I see the following options:

  • disable the tests alltogether (not a good idea since most pass)
  • remove tests that involves invalid (get rid of math.h as well)
  • #ifdef the invalid tests away for Msvc

I slightly prefer the second option over the third. Invalid will still work on those systems that pick it up, but it won't be documented as working by any tests. I would like to see sqrt() and math.h gone.

@arstrube
Copy link
Contributor

oooh. But just realized if we act on it and don't test it we will decrease coverage :-((

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.

None yet

4 participants