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

DoubleBits, avoid garbage value #252

Closed
wants to merge 3 commits into from

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Dec 2, 2019

After the merging of the PR #238, there remain only one issue checking with xcode static analyser (xcode 10.2.1):

/geos/src/index/quadtree/DoubleBits.cpp:144:11: warning: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage
    xBits &= mask;
    ~~~~~ ^
/geos/src/index/quadtree/DoubleBits.cpp:152:19: warning: The left operand of '&' is a garbage value
    return (xBits & mask) != 0 ? 1 : 0;
            ~~~~~ ^
2 warnings generated.

This fix addresses this.

@dbaston
Copy link
Member

dbaston commented Dec 10, 2019

I don't think this is the right fix. Isn't 0 just a different garbage value? xBits isn't initialized when ASSUME_IEEE_DOUBLE isn't defined, so we probably shouldn't be reading from it in that case either. Or we could simply do a static assert for IEEE compliance and remove the other code pathways.

@nilason
Copy link
Contributor Author

nilason commented Dec 10, 2019

You make a very good point there. Indeed, __STDC_IEC_559__ is not defined by all compilers and isn't a fool-proof check for IEEE compliance. This is the case with clang (thus the warnings).

Or we could simply do a static assert for IEEE compliance and remove the other code pathways.

This seems to me to be the preferable solution, although I'm afraid I cannot help out on that.

@nilason
Copy link
Contributor Author

nilason commented Dec 12, 2019

Couldn't resist attempting to fix this using static_assert, with latest pushed commit. Seems to be fine with CI's.

(note: if it comes to merge, I need to clean up commit history before)

@pramsey
Copy link
Member

pramsey commented Apr 30, 2020

So, coming back to this, the final patch is brutal in its simplicity but it also forecloses working on any platform that doesn't have ieee754... which is... which platforms? I mean, I'd like to know what doors we're closing with this. Also, we're only closing them for what? To shut up a static analyzer?

@abellgithub
Copy link
Contributor

It looks like much of this code isn't used, and what is used can be replaced by library calls, I think.

@pramsey pramsey mentioned this pull request Apr 30, 2020
@pramsey
Copy link
Member

pramsey commented Apr 30, 2020

Seems like, trying in #317

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