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

Support all CMake booleans #437

Merged
merged 1 commit into from Sep 12, 2018

Conversation

Projects
None yet
3 participants
@HadrienG2
Contributor

HadrienG2 commented Sep 11, 2018

CMake supports other Boolean truth values than "ON" and "OFF", including for example "TRUE"/"FALSE", "0"/"1", and "YES"/"NO". Therefore, hardcoding boolean comparisons to "ON"/"OFF" will cause subtle problems when CMake is used in an unintended way and is best avoided.

BEGINRELEASENOTES

  • Remove if string equals ON/OFF in cmake IF statements and check default CMake truth values

ENDRELEASENOTES

@petricm

This comment has been minimized.

Member

petricm commented Sep 11, 2018

thank you for your contribution and I see your point, but this in it's current state breaks everything. Please fix the PR.

@HadrienG2

This comment has been minimized.

Contributor

HadrienG2 commented Sep 11, 2018

You're right, I missed some use of undefined variables. Adding a check of defined-ness...

@HadrienG2 HadrienG2 changed the title from Support all CMake booleans to [WIP] Support all CMake booleans Sep 11, 2018

@petricm petricm added the cleanup label Sep 11, 2018

@HadrienG2

This comment has been minimized.

Contributor

HadrienG2 commented Sep 11, 2018

Ok, this version looks good.

@HadrienG2 HadrienG2 changed the title from [WIP] Support all CMake booleans to Support all CMake booleans Sep 11, 2018

@petricm

This comment has been minimized.

Member

petricm commented Sep 11, 2018

thank you for fixing it, please just squash the commits.

@HadrienG2 HadrienG2 force-pushed the HadrienG2:support-all-cmake-bools branch from f46e679 to 2828ed9 Sep 11, 2018

@HadrienG2

This comment has been minimized.

Contributor

HadrienG2 commented Sep 11, 2018

Done!

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Sep 11, 2018

Marko,
if you agree with this solution,please merge.

@petricm petricm merged commit f02c850 into AIDASoft:master Sep 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment