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

Updated/fixed string comparisons in Config.cmake #1102

Merged
merged 1 commit into from Jul 17, 2016

Conversation

Projects
None yet
5 participants
@MarioLiebisch
Member

MarioLiebisch commented Jun 20, 2016

SFML so far used string comparisons like ${CMAKE_SYSTEM_NAME} MATCHES "Windows".
This works, but only because there's a variable WINDOWS having the exact same content (Windows).

More information and a similar issue can be found in this SO thread.

@MarioLiebisch MarioLiebisch added this to the 2.4 milestone Jun 20, 2016

@MarioLiebisch MarioLiebisch self-assigned this Jun 20, 2016

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jun 22, 2016

Member

👍

Member

mantognini commented Jun 22, 2016

👍

@@ -8,15 +8,15 @@ if(${CMAKE_SYSTEM_NAME} MATCHES "Windows")
# detect the architecture (note: this test won't work for cross-compilation)
include(CheckTypeSize)
check_type_size(void* SIZEOF_VOID_PTR)
if("${SIZEOF_VOID_PTR}" STREQUAL "4")
if(${SIZEOF_VOID_PTR} STREQUAL "4")

This comment has been minimized.

@eXpl0it3r

eXpl0it3r Jun 22, 2016

Member

Is that really correct? (Any references?)
I mainly ask because I remember that this was a bit of a strange CMake thing with quotes and STREQUAL (reminds me of Batch files...).

@eXpl0it3r

eXpl0it3r Jun 22, 2016

Member

Is that really correct? (Any references?)
I mainly ask because I remember that this was a bit of a strange CMake thing with quotes and STREQUAL (reminds me of Batch files...).

This comment has been minimized.

@MarioLiebisch

MarioLiebisch Jun 22, 2016

Member

As far as I know, yes. STREQUAL will implicitly interpret both sides as a string and I didn't get any warnings or errors, so I assume it's working.

@MarioLiebisch

MarioLiebisch Jun 22, 2016

Member

As far as I know, yes. STREQUAL will implicitly interpret both sides as a string and I didn't get any warnings or errors, so I assume it's working.

This comment has been minimized.

@JojOatXGME

JojOatXGME Nov 17, 2017

I accidentally found the corresponding pull request #1102. I'm not sure if I should write comments on closed tickets or merged commits but maybe someone want to lern from it.

The proposed change does not solve the problem. After applying this change, the condition would even be true for SIZEOF_VOID_PTR == "1;OR;ABC" since it is unrolled to if(1 OR ABC STREQUAL 4).

However, the pull request was right that there existed a flaw which is not been fixed yet. CMake is quite strange in some cases and there does not seem to be an proper solution. The statement X STREQUAL Y does actually check whether there is a variable X or Y. In this case, it will use the content of the variable, otherwise it would just use X or Y. It is described in the documentation. This means you could use if(SIZEOF_VOID_PTR STREQUAL "4") (without ${}) to avoid double-dereferencing the variable. However, when you would compare it to a string instead of a number (4), that string might be dereferenced as well. MATCHES does not dereference the right operand but it does not seem to support exact matches. So, there does not seem to be a proper solution without adding extra lines for every if. However, I guess that this strange behavior has not caused a problem in your project so far.

Luckily, there has been added a proper solution through policy CMP0054 in CMake 3.1.^^

@JojOatXGME

JojOatXGME Nov 17, 2017

I accidentally found the corresponding pull request #1102. I'm not sure if I should write comments on closed tickets or merged commits but maybe someone want to lern from it.

The proposed change does not solve the problem. After applying this change, the condition would even be true for SIZEOF_VOID_PTR == "1;OR;ABC" since it is unrolled to if(1 OR ABC STREQUAL 4).

However, the pull request was right that there existed a flaw which is not been fixed yet. CMake is quite strange in some cases and there does not seem to be an proper solution. The statement X STREQUAL Y does actually check whether there is a variable X or Y. In this case, it will use the content of the variable, otherwise it would just use X or Y. It is described in the documentation. This means you could use if(SIZEOF_VOID_PTR STREQUAL "4") (without ${}) to avoid double-dereferencing the variable. However, when you would compare it to a string instead of a number (4), that string might be dereferenced as well. MATCHES does not dereference the right operand but it does not seem to support exact matches. So, there does not seem to be a proper solution without adding extra lines for every if. However, I guess that this strange behavior has not caused a problem in your project so far.

Luckily, there has been added a proper solution through policy CMP0054 in CMake 3.1.^^

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 6, 2016

Member

👍

Member

binary1248 commented Jul 6, 2016

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 9, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Jul 9, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Updated/fixed string comparisons in Config.cmake
SFML so far used `${CMAKE_SYSTEM_NAME} MATCHES "Windows"`. This works, but only because there's a variable `WINDOWS` having the exact same content (`Windows`).

[More information and a similar issue can be found in this SO thread.](http://stackoverflow.com/questions/21995777/cygwins-cmake-does-not-match-for-cmake-system-name)

@eXpl0it3r eXpl0it3r merged commit ba9383f into master Jul 17, 2016

13 checks passed

debian-gcc-64 Build #156 done.
Details
freebsd-gcc-64 Build #156 done.
Details
osx-clang-el-capitan Build #36 done.
Details
static-analysis Build #156 done.
Details
windows-gcc-492-tdm-32 Build #41 done.
Details
windows-gcc-492-tdm-64 Build #41 done.
Details
windows-gcc-530-mingw-32 Build #41 done.
Details
windows-gcc-530-mingw-64 Build #41 done.
Details
windows-vc11-64 Build #157 done.
Details
windows-vc12-32 Build #157 done.
Details
windows-vc12-64 Build #155 done.
Details
windows-vc14-32 Build #156 done.
Details
windows-vc14-64 Build #158 done.
Details

@eXpl0it3r eXpl0it3r deleted the bugfix/cmake-config-matching branch Jul 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment