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

WIP: Bugfix: fix issue 145 compile on VS 2022 #146

Merged

Conversation

burnpanck
Copy link
Contributor

This PR provides a workaround around a compiler bug, which seems to have appeared in VS 2022. It appears that the bug is only triggered with /permissive-, which is enabled by default when using /std:c++20. As I was unable to make CMAKE set the former, I simply set the latter in the accompanying CI tests.

In this version of the PR, the workaround is hidden behind a FOONATHAN_SFINAE_WORKAROUND guard, which for now is implemented unconditionally, as I haven't fully determined what versions and compiler flag combinations really trigger the issue. Since you have used std::declval in other places within FOONATHAN_SFINAE, maybe the workaround could actually be used unconditionally everywhere, and the guard removed? Let me know.

Copy link
Owner

@foonathan foonathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you; see comments.

.github/workflows/main_ci.yml Outdated Show resolved Hide resolved
.github/workflows/main_ci.yml Outdated Show resolved Hide resolved
include/foonathan/memory/allocator_storage.hpp Outdated Show resolved Hide resolved
include/foonathan/memory/config.hpp Outdated Show resolved Hide resolved
@foonathan
Copy link
Owner

Ignore the MacOS failures; I'll fix.

@foonathan foonathan force-pushed the bugfix/fix-issue-145-compile-on-msvc-2022 branch from 852c244 to 10452d3 Compare November 25, 2022 18:31
…m:burnpanck/memory into bugfix/fix-issue-145-compile-on-msvc-2022
@burnpanck burnpanck force-pushed the bugfix/fix-issue-145-compile-on-msvc-2022 branch from 434dc58 to 9d2b80e Compare November 27, 2022 16:35
@foonathan foonathan merged commit 3144037 into foonathan:main Nov 28, 2022
@foonathan
Copy link
Owner

Thank you.

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.

2 participants