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

Remove support for long double #597

Merged
merged 6 commits into from
Dec 15, 2017
Merged

Conversation

antoinefalisse
Copy link
Member

@antoinefalisse antoinefalisse commented Dec 13, 2017

This PR aims to remove support for long double ( #596 ). I have removed pieces of code where long doubles were used and adjusted comments where appropriate. I am not sure that long doubles had to be removed everywhere though. For instance, in conjugate.h, I removed definitions of operators involving long doubles but this may not be the right approach. I did not touch the definitions of the SimTK_NICETYPENAME_LITERAL involving long doubles in common.h (I did not touch TestNiceTypeName.cpp either). Please let me know if this is the intended approach or if I should proceed differently.

Reviewers: @sherm1 and @chrisdembia


This change is Reviewable

@chrisdembia
Copy link
Member

@sherm1, here are the places where long double remains:

SimTKcommon/include/SimTKcommon/Constants.h
37:values in long double precision.
51:macros in long double precision. By using very high precision we ensure 
52:sufficient accuracy for any IEEE long double precision implementation (they 
120:written here in maximal (long double) precision. (These values were generated 
124:the compiler will properly calculate a long double result with no runtime cost.

SimTKcommon/include/SimTKcommon/internal/common.h
918:SimTK_NICETYPENAME_LITERAL(long double);
922:SimTK_NICETYPENAME_LITERAL(std::complex<long double>); 

SimTKcommon/Mechanics/include/SimTKcommon/Orientation.h
57:// long double results which you can cast to smaller sizes if you want.

SimTKcommon/tests/BNTTest.cpp
335:        (long double)NTraits<float>::getEps(), 
336:        (long double)NTraits<double>::getEps());
339:        (long double)NTraits<float>::getSqrtEps(), 
340:        (long double)NTraits<double>::getSqrtEps());
343:        (long double)NTraits<float>::getSignificant(), 
344:        (long double)NTraits<double>::getSignificant());
347:        (long double)NTraits<float>::getTiny(), 
348:        (long double)NTraits<double>::getTiny());

SimTKcommon/tests/TestNiceTypeName.cpp
186:    SimTK_TEST(NiceTypeName<long double>::namestr() == "long double");
191:    SimTK_TEST(NiceTypeName<std::complex<long double>>::namestr() 
192:               == "std::complex<long double>");
206:    SimTK_TEST(NiceTypeName<std::complex<long double>>::xmlstr() 
207:               == "std::complex{long double}");

SimTKmath/Integrators/src/CPodes/sundials/include/sundials/sundials_types.h
19: * float, double or long double.
25: * The legal types for realtype are float, double and long double.
35: * expands to 1.0F. If realtype is defined as a long double,
78:typedef long double realtype;

SimTKmath/Integrators/src/CPodes/sundials/INSTALL_NOTES.txt
283:  (float C-type) if ARG=single, or as a long double C-type if ARG=extended.

SimTKmath/Integrators/src/CPodes/sundials/tests/pend.c
39:  printf("long double  %d\n",(int)sizeof(long double));

I agree with leaving in SimTK_NICETYPENAME_LITERAL(long double) but @sherm1 will have to give input about conjugate<long double>.

I looked through all the changes in the PR and they look good to me.


Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Dec 14, 2017

This is great! Thanks @antoinefalisse. It is really nice to see all that unneeded code go away. It really cuts down on the complexity and future maintenance headache.

One minor request in BNTTest.cpp, otherwise :lgtm_strong:

+@sherm1 +@chrisdembia (this syntax tells Reviewable to require lgtm's from us - you can use it to request reviewers)


Reviewed 21 of 21 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


SimTKcommon/tests/BNTTest.cpp, line 348 at r1 (raw file):

    printf("Tiny f=%.16Lg d=%.16Lg\n",
        (long double)NTraits<float>::getTiny(), 
        (long double)NTraits<double>::getTiny());

BTW Please change these to (double) and remove the "L" from "Lg" in the four printf format strings - no need for long double anymore.


Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Dec 14, 2017

P.S. @chrisdembia -- thanks for the long double report . Very helpful!


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@1fb07d6). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #597   +/-   ##
=========================================
  Coverage          ?   49.85%           
=========================================
  Files             ?      557           
  Lines             ?    70076           
  Branches          ?        0           
=========================================
  Hits              ?    34939           
  Misses            ?    35137           
  Partials          ?        0
Impacted Files Coverage Δ
SimTKcommon/Scalar/include/SimTKcommon/Scalar.h 100% <ø> (ø)
SimTKcommon/include/SimTKcommon/internal/String.h 2.43% <ø> (ø)
...mon/SmallMatrix/include/SimTKcommon/internal/Vec.h 10.84% <ø> (ø)
...Matrix/include/SimTKcommon/internal/MatrixHelper.h 30% <ø> (ø)
...mon/SmallMatrix/include/SimTKcommon/internal/Mat.h 8.3% <ø> (ø)
...on/Scalar/include/SimTKcommon/internal/conjugate.h 0% <ø> (ø)
...mmon/Scalar/include/SimTKcommon/internal/NTraits.h 14.75% <ø> (ø)
SimTKcommon/include/SimTKcommon/internal/common.h 50% <ø> (ø)
SimTKcommon/Polynomial/src/cpoly.cpp 88.07% <ø> (ø)
SimTKcommon/Polynomial/src/rpoly.cpp 90.41% <ø> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fb07d6...c2b20b9. Read the comment docs.

@antoinefalisse
Copy link
Member Author

@sherm1, @chrisdembia, I have made the changes in BNTTest so this PR should now be ready. Thanks!


Review status: 20 of 21 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Dec 15, 2017

Thanks, @antoinefalisse!


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sherm1 sherm1 merged commit 59e5b9e into simbody:master Dec 15, 2017
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