Correctly handle float division in Scalar
ClosedPublic

Authored by tromey on Mar 20 2018, 11:11 AM.

Details

Summary

Scalar's operator/ has an inverted test for the LHS being zero.

Diff Detail

Repository
rL LLVM
tromey created this revision.Mar 20 2018, 11:11 AM

I've also noticed that int->float conversion in Scalar seems incorrect. At least, Scalar.h claims that Scalar follows C promotion rules, but int->float conversion is done using bitwise reinterpretation rather than preserving the value. I have a patch for this, but I don't know whether or not it's correct to change.

clayborg accepted this revision.Mar 21 2018, 9:32 AM
This revision is now accepted and ready to land.Mar 21 2018, 9:32 AM
clayborg requested changes to this revision.Mar 21 2018, 9:41 AM

Actually, we should add a test for this to ensure this doesn't regress.

This revision now requires changes to proceed.Mar 21 2018, 9:41 AM

Seems like a unit test would work well in this case.

davide added a subscriber: davide.Mar 21 2018, 10:25 AM

Yes, this needs a test. Thanks!

Bonus points for catching any other conversion errors that don't match the current C/C++ specs. The unit tests should be an easy place to test these kinds of things.

tromey updated this revision to Diff 139616.Mar 23 2018, 11:10 AM

Add unit test

davide accepted this revision.Mar 23 2018, 11:25 AM

LGTM, thanks. Do you have commit access or you need somebody to commit this on your behalf?

LGTM, thanks. Do you have commit access or you need somebody to commit this on your behalf?

I do not have commit access.

clayborg accepted this revision.Mar 23 2018, 1:27 PM
This revision is now accepted and ready to land.Mar 23 2018, 1:27 PM

Allright, we got this one.

davide@Davidinos-Mac-Pro ~/w/l/l/lldb> git llvm push
Pushing 1 commit:

55f24c19d1c [Core] Correctly handle float division in Scalar.

Sending lldb/trunk/source/Core/Scalar.cpp
Sending lldb/trunk/unittests/Core/ScalarTest.cpp
Transmitting file data ..done
Committing transaction...
Committed revision 328649.
Committed 55f24c19d1c to svn.

This revision was automatically updated to reflect the committed changes.