Skip to content

Don't check bitness in CMakeLists.txt #192

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

Closed
ryandesign opened this issue Apr 9, 2015 · 4 comments
Closed

Don't check bitness in CMakeLists.txt #192

ryandesign opened this issue Apr 9, 2015 · 4 comments
Assignees
Milestone

Comments

@ryandesign
Copy link

Hello, I am the maintainer of tidy in MacPorts. In Tidy 4.9.24 at least, your CMakeLists.txt attempts to determine whether the machine is 32-bit or 64-bit. Please don't do that in CMakeLists.txt because doing so there is not compatible with OS X universal builds, which are desirable. In a universal build, one runs cmake a single time, and then builds simultaneously for 32-bit and 64-bit (or simultaneously for PowerPC and Intel, or simultaneously for 3 or 4 combinations of these), so you cannot know at the time that cmake is run what the bitness (or endianness) is.

Instead, use preprocessor defines such as __LP64__ in your code files if you need to differentiate between 32-bit and 64-bit.

@geoffmcl
Copy link
Contributor

geoffmcl commented Apr 9, 2015

@ryandesign, well in the CMakeLists.txt, the only check for compiler bitness, is -

if (CMAKE_SIZEOF_VOID_P EQUAL 8)
    message(STATUS "*** Have SIZEOF void * = 8, so 64-bit")
    set( IS_64_BIT 1 )
else ()
    message(STATUS "*** SIZEOF void * != 8, so not 64-bit")
endif ()

And this result is ONLY used inside a (WIN32 AND MSVC) block, like -

if(WIN32 AND MSVC)
    ...
    if (IS_64_BIT)
        set( MSVC_FLAGS "${MSVC_FLAGS} -DWIN64" )
    endif ()
    ...
else ()

So you can see it is ONLY used to set -DWIN64 in the MSVC Windows build.

There is NO check for endianness!

No other use is made of this flag, and can not see how the cmake testing of `CMAKE_SIZEOF_VOID_P' would influence your OS X universal build?

However, if it does somehow cause a problem, then I suppose I could move the test to inside the WIN32 AND MSVC block... but thinking more about this I am not even sure WIN64 need be defined, but would need to check more on that...

But if you get a chance please re-check and advise, as we cetainly do not want to cause an OS X universal build problem... Tidy should compile and run everywhere ;=))

@geoffmcl geoffmcl added this to the 5.0.0 milestone Apr 9, 2015
@geoffmcl geoffmcl self-assigned this Apr 9, 2015
@ryandesign
Copy link
Author

I saw the output *** SIZEOF void * != 8, so not 64-bit in my 64-bit/32-bit universal build, assumed this would be a problem, and worked around it. I did not attempt to look at your code to see where that determination was used. If it is only used on Windows, then I'll add a patch to MacPorts to not run that check, so that the *** SIZEOF void * != 8, so not 64-bit output doesn't appear and doesn't confuse anyone else.

@geoffmcl
Copy link
Contributor

@ryandesign, have now had time to check for the 'need' in a windows build...

It seems there is NO need to check the architecture in windows either, so have removed the 'check' completely, and the cmake message output, to avoid confusion. It seems it was only there due to a cut-and-paste from another project when I created the initial CMakeList.txt file.

Just for information, in windows, if there was a need to check 32 vs 64 bit, then the correct macro is #ifdef _WIN64. This is defined by the MSVC 64-bit compiler...

Hope this closes this issue. Thanks for reporting...

@ryandesign
Copy link
Author

Great! Thank you.

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

No branches or pull requests

2 participants