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

patch to support PowerPC and Windows 32 bits #7635

Merged
merged 2 commits into from
Aug 14, 2023

Conversation

lrineau
Copy link
Member

@lrineau lrineau commented Aug 3, 2023

Summary of Changes

Patch required for the acceptance of CGAL-5.6 in vcpkg. The CI system tests on x86_windows (32 bits), and there was an error:

D:\installed\x86-windows\include\CGAL/cpp_float.h(30): error C3861: '_BitScanForward64': identifier not found
D:\installed\x86-windows\include\CGAL/cpp_float.h(42): error C3861: '_BitScanReverse64': identifier not found

See microsoft/vcpkg#32896 (comment)

The solution is do disable support for Boost MP on Windows 32 bits. That patch has been tested with the vcpkg CI, and it worked.

Release Management

@sloriot
Copy link
Member

sloriot commented Aug 3, 2023

Shouldn't you patch Number_types/CGAL/include/boost_mp.h instead ?

Currently the condition is

    (!defined _MSC_VER || BOOST_VERSION >= 107000)
#define CGAL_USE_BOOST_MP 1

Maybe you can use _WIN32 here?

@lrineau
Copy link
Member Author

lrineau commented Aug 3, 2023

@lrineau
Copy link
Member Author

lrineau commented Aug 3, 2023

Shouldn't you patch Number_types/CGAL/include/boost_mp.h instead ?

Maybe. That would fix also the bug for users not using our CMake scripts.

I was more comfortable with a patch in CMake.

Currently the condition is

    (!defined _MSC_VER || BOOST_VERSION >= 107000)
#define CGAL_USE_BOOST_MP 1

Maybe you can use _WIN32 here?

_WIN32 is not a right choice because it does not really discriminate 32/64 bits:

https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?

_WIN32 Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise, undefined.

Maybe !_WIN64 instead.

@lrineau
Copy link
Member Author

lrineau commented Aug 3, 2023

I have a new patch. Please review cf0c6c0.

@lrineau lrineau changed the title patch to support Windows 32 bits patch to support PowerPC and Windows 32 bits Aug 3, 2023
@lrineau lrineau added this to the 5.6.1 milestone Aug 3, 2023
@lrineau
Copy link
Member Author

lrineau commented Aug 3, 2023

@mglisse
I think we could support Windows 32 bits using that kind of code:

#ifdef _WIN64
  _BitScanForward64(&r, x);
#else
  if (_BitScanForward(&r, (uint32_t)(x >> 32))) {
    r += 32;
  } else {
    _BitScanForward(&r, (uint32_t)x);
  }
#endif

See https://github.com/janestreet/base/blob/02cf612470d5d37e0e9a704cdcecd5306ff196b7/src/int_math_stubs.c#L40-L52

Would the rest of CGAL/cpp_float.h work?

@sloriot
Copy link
Member

sloriot commented Aug 14, 2023

Successfully tested in CGAL-6.0-Ic-39

@sloriot sloriot merged commit d0b188b into CGAL:5.6.x-branch Aug 14, 2023
8 checks passed
@sloriot sloriot deleted the CGAL-Windows-32bits-GF branch August 14, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants