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

[eastl] Fix error C2338 #21538

Merged
merged 4 commits into from
Feb 9, 2022
Merged

Conversation

Cheney-W
Copy link
Contributor

  • What does your PR fix?

    In an internal version of Visual Studio, eastl install failed with following error:
    internal\atomic\atomic_integral.h(330): error C2338: eastl::atomic<T> atomic macro not implemented!

    This issue could be fixed by adding the compile option: /Zc:static_assert- or modifying the source, I added this patch only as a temporary workaround, the specific fix still needs to be discussed with upstream.

No feature need to be tested.

@Cheney-W Cheney-W added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Nov 19, 2021
@Cheney-W Cheney-W marked this pull request as ready for review November 22, 2021 02:51
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Nov 29, 2021
@BillyONeal
Copy link
Member

What is the behavior of adding /Zc:static_assert- with older compilers?

@BillyONeal
Copy link
Member

I observe that /Zc:static_assert- is not documented: https://docs.microsoft.com/en-us/cpp/build/reference/zc-conformance?view=msvc-170

@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Nov 29, 2021
@Cheney-W
Copy link
Contributor Author

  1. I have compared one by one the output of this port between the older compiler and the latest compiler with /Zc:static_assert- option, I didn't notice any difference.
  2. This option is added by Jonathan Caves, please check the related commit for more info.

@BillyONeal
Copy link
Member

I asked Jonathan Caves to comment.

@BillyONeal BillyONeal added the depends:upstream-changes Waiting on a change to the upstream project label Dec 13, 2021
@BillyONeal
Copy link
Member

(I'm not sure whether 'author response' or 'upstream changes' convey the right tone here but...)

@Cheney-W Cheney-W marked this pull request as draft December 29, 2021 02:38
@Cheney-W
Copy link
Contributor Author

Avoid affecting the PR age, converted this PR to draft.

@BillyONeal
Copy link
Member

I know we talked a lot about this change out-of-band; to summarize what I think needs to happen:

  • Given that there is no documented behavior for what the compiler does on unrecognized /Zc flags and the existence of clang-cl and friends, I think this needs to be guarded for versions of the compiler new enough to support it.
  • Documentation for the new /Zc switch needs to be added to MSDN/docs.microsoft.com/etc.

@JackBoosY
Copy link
Contributor

Ping @Cheney-W

@Cheney-W
Copy link
Contributor Author

Cheney-W commented Feb 7, 2022

The /Zc:static_assert- option is now recognized in the latest VS2022 17.1.0 Preview 5, but this version should not be able to be used as a judgment node for implementing version protection.
We may have to wait for VS2022 17.1 to be released.

@BillyONeal What do you think about this?

@LilyWangLL LilyWangLL mentioned this pull request Feb 7, 2022
@BillyONeal
Copy link
Member

@Cheney-W As long as the version check can be done in CMake world I think it's fine without waiting for 17.1. Perhaps CMAKE_C_COMPILER_VERSION/CMAKE_CXX_COMPILER_VERSION could work?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/eastl/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

@Cheney-W Cheney-W removed the depends:upstream-changes Waiting on a change to the upstream project label Feb 8, 2022
@Cheney-W Cheney-W marked this pull request as ready for review February 8, 2022 08:49
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 9, 2022
@ras0219-msft ras0219-msft merged commit dd4c769 into microsoft:master Feb 9, 2022
@ras0219-msft
Copy link
Contributor

Thanks!

@Cheney-W Cheney-W deleted the Dev/Cheney/eastl branch August 30, 2022 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants