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

Use _BitScanForward and _BitScanReverse so x86 builds still compile #7896

Closed
wants to merge 1 commit into from

Conversation

dlrdave
Copy link

@dlrdave dlrdave commented Nov 29, 2023

Summary of Changes

Use _BitScanForward and _BitScanReverse so x86 builds still compile

More precisely, avoid using _BitScanForward64 and _BitScanReverse64 since they do not exist in the x86 compiler intrinsics.

https://learn.microsoft.com/en-us/cpp/intrinsics/bitscanforward-bitscanforward64?view=msvc-170

Avoids these compiler errors:
CGAL\include\CGAL/cpp_float.h(30): error C3861: '_BitScanForward64': identifier not found
CGAL\include\CGAL/cpp_float.h(42): error C3861: '_BitScanReverse64': identifier not found

Release Management

  • Affected package(s): Number_types
  • Issue(s) solved (if any): compiler errors encountered on Windows x86 builds
  • License and copyright ownership: unchanged

More precisely, avoid using _BitScanForward64 and _BitScanReverse64
since they do not exist in the x86 compiler intrinsics.

https://learn.microsoft.com/en-us/cpp/intrinsics/bitscanforward-bitscanforward64?view=msvc-170
@lrineau
Copy link
Member

lrineau commented Nov 29, 2023

Hi @dlrdave. Do you still have Windows 32 bits builds of products using CGAL?

Is that for Neocis?

@dlrdave
Copy link
Author

dlrdave commented Nov 29, 2023

Hi @dlrdave. Do you still have Windows 32 bits builds of products using CGAL?

Is that for Neocis?

Yes, this is a patch we are using at Neocis to keep 32 bit builds working. We are not actively using CGAL in any 32-bit products, but this patch is easier for us than the amount of work it would take to exclude single packages from our 32-bit library building pipeline.

@lrineau
Copy link
Member

lrineau commented Nov 29, 2023

Can you please have a look at the PR #7635 and in particular that comment #7635 (comment)?

I am surprised that you have build issues with Windows x86, because PR #7635 disabled the support of Boost MP with that platform.

@sloriot
Copy link
Member

sloriot commented Nov 29, 2023

Can you please have a look at the PR #7635 and in particular that comment #7635 (comment)?

I am surprised that you have build issues with Windows x86, because PR #7635 disabled the support of Boost MP with that platform.

It's not yet released, it will be part of 5.6.1

@dlrdave
Copy link
Author

dlrdave commented Nov 29, 2023

It's not yet released, it will be part of 5.6.1

:-)

OK, thanks. You can reject this PR then, and we will pick up 5.6.1 when it comes out, or hopefully even update to 6 in the near future instead.

@dlrdave
Copy link
Author

dlrdave commented Nov 29, 2023

Closing this PR, since an equivalent fix is already slated to be included in the upcoming 5.6.1 release.

@dlrdave dlrdave closed this Nov 29, 2023
@lrineau
Copy link
Member

lrineau commented Nov 29, 2023

Closing this PR, since an equivalent fix is already slated to be included in the upcoming 5.6.1 release.

Actually, that is not completely equivalent:

That is quite different. If Neocis still use Windows x86 builds, then maybe the CGAL project should start again to test on that platform. Please tell us.

@dlrdave
Copy link
Author

dlrdave commented Nov 29, 2023

If Neocis still use Windows x86 builds, then maybe the CGAL project should start again to test on that platform. Please tell us.

I would rather do the work of excluding CGAL from our x86 pipeline than impose the testing burden of such an old platform on the CGAL project. Thanks, but we can patch for now, and exclude in a little while. We only really need CGAL support on 64-bit platforms.

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.

3 participants