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

Improve variable conversion & comparison handling as well as CMake version compatibility #671

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

3manifold
Copy link
Contributor

Importing quill source code as part of a larger project can reveal points of improvement mainly pointed out by inherited compiler flags. Fixes include:

  • Tolerate float comparison when using equality operator.
# error
comparing floating point with == or != is unsafe [-Werror,-Wfloat-equal]
  • Improve implicit conversion to avoid lose of precision (hopefully multiplication result falls in the original author's error tolerance intention).
# warning
  implicit conversion from 'const integer_type' (aka 'const unsigned long') to 'double' may lose precision [-Werror,-Wimplicit-int-float-conversion]
  • Modern CMake versions (e.g. 3.31.5) can reveal obsolete CMake policy incompatibility. Changing minimum version to 3.10 alleviates these warnings. Worth noting that current version 3.8 dates back to 2017.
# warning message
CMake Deprecation Warning at extern/quill/CMakeLists.txt:1 (cmake_minimum_required):
  Compatibility with CMake < 3.10 will be removed from a future version of
  CMake.
<...>

@odygrd
Copy link
Owner

odygrd commented Feb 14, 2025

Hey, thanks for the PR. Which line of code is giving you this warning ? And on which compiler? I would prefer to fix it rather than silencing it

‘comparing floating point with == or != is unsafe [-Werror,-Wfloat-equal]’

@3manifold
Copy link
Contributor Author

Hi, it is clang 19.1.7 and line if (value == 0). The code will fail to build if one uses -Wfloat-equal.

@odygrd
Copy link
Owner

odygrd commented Feb 14, 2025

Thanks, I have silenced the warning in this PR, so let's not add the Wfloat-equal flag please

#672

if we add a compilation flag to an interface target that flag or definition will propagate to all translation units that include the headers from that interface target

Co-authored-by: Odysseas Georgoudis <[email protected]>
@odygrd odygrd merged commit d342624 into odygrd:master Feb 14, 2025
25 checks passed
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.

2 participants