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

QNANO_PROPERTY(double...) -> "comparing floating point with == or != is unsafe." #31

Open
NielsMayer opened this issue Sep 25, 2018 · 4 comments

Comments

@NielsMayer
Copy link
Contributor

NielsMayer commented Sep 25, 2018

On upgrading to QtCreator 4.7.1 and Qt5.11.2, I noticed massive numbers of clang-code-model warnings appearing in qnanopainter related source-code.

In particular, when using QNANO_PROPERTY(double,....) one is greeted by warnings that "comparing floating point with == or != is unsafe." In researching this issue, I was greeted by the sound of a can of worms opening: https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/

In the above macro, the problem code in question is:
void setter(type const &v) { if(v == variable) return; variable = v; emit getter##Changed(); update(); }

What is the "correct" way of eliminating the above warning and any "danger" concerns it may cause? How unsafe is this in practice? I notice the only instances of such code in qnanopainter source are commented out https://github.com/QUItCoding/qnanopainter/blob/master/examples/gallery/qnanopainter_features/src/galleryitem.h and that custom code is used for examples where Animation float values are passed in from QML.

@QUItCoding
Copy link
Owner

As said that helper macro isn't used in QNanoPainter sources currently. QNANO_PROPERTY does reduce boilerplate code for simple setter/getter properties, while cons is that Qt Creator code completion doesn't work for methods & variables defined using it.

That comparison isn't really unsafe, worst case scenario is that *Changed() signal will get called even when value didn't actually change as comparison thought they are different. So you can just ignore the warnings or not use QNANO_PROPERTY for float properties. Floating point specific version of that macro could be implemented with qFuzzyCompare & qFuzzyIsNull but not sure if that is needed...

@NielsMayer
Copy link
Contributor Author

NielsMayer commented Sep 26, 2018

The helper macro is used here in the qnanopainter source:
https://github.com/QUItCoding/qnanopainter/blob/master/examples/gallery/qnanopainter_features/src/galleryitem.h#L14

The equivalent code from the macro is present with the alternative macro version commented out:

//Alternatively could just use:
//QNANO_PROPERTY(float, m_animationTime, animationTime, setAnimationTime)
//QNANO_PROPERTY(float, m_animationSine, animationSine, setAnimationSine)

In the equivalent alternative code as would be generated by the macro, the "unsafe" warning is also emitted for the following two cases -- again within current qnanopainter source code:
https://github.com/QUItCoding/qnanopainter/blob/master/examples/gallery/qnanopainter_features/src/galleryitem.h#L30
https://github.com/QUItCoding/qnanopainter/blob/master/examples/gallery/qnanopainter_features/src/galleryitem.h#L38

@NielsMayer
Copy link
Contributor Author

NielsMayer commented Dec 12, 2018

In reply to @QUItCoding comment from commit commentary in #32:

"By the way, tried also using qFuzzyCompare() for QNANO_PROPERTY but macros are a bit too dummy for that... Possibly would need separate QNANO_FLOAT_PROPERTY macro for float properties."

A separate QNANO_FLOAT_PROPERTY (also probably QNANO_DOUBLE_PROPERTY) would at least silence the warnings and be safer and less bug-inducing.

Couldn't QNANO_PROPERTY(_,_) special-case match for 'double' or 'float' and in turn invoke QNANO_FLOAT_PROPERTY(_) or QNANO_DOUBLE_PROPERTY(_) ?

NielsMayer referenced this issue Dec 12, 2018
And use qFuzzyCompare as discussed in issue #32.
@QUItCoding
Copy link
Owner

In theory yes, but I'm not fluent with macros to implement that =)

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

No branches or pull requests

2 participants