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

Fix constexpr on clang for eastl #19554

Closed
wants to merge 3 commits into from
Closed

Conversation

Kalixio
Copy link
Contributor

@Kalixio Kalixio commented Sep 1, 2023

Specify library name and version: eastl/3.21.12

When building with clang 16, it was not possible to use constexpr with eastl
This is due to GNUC being defined on clang and EA_COMPILER_VERSION being less than 9000

With clang, EA_COMPILER_VERSION is defined to:

#define EA_COMPILER_VERSION (__clang_major__ * 100 + __clang_minor__)

see https://github.com/electronicarts/EABase/blob/521cb053d9320636f53226ffc616216cf532f0ef/include/Common/EABase/config/eacompiler.h#L341C53-L341C53

I propose to exclude clang from

	#elif defined(__GNUC__) && (EA_COMPILER_VERSION < 9000)   // Before GCC 9.0
		#define EA_CPP14_CONSTEXPR  // not supported
		#define EA_NO_CPP14_CONSTEXPR 

Since it only applies to gcc before 9.0, according to the comment

Also, added a constexpr test case in the test package

Tested locally on MSVC 19.37.32822.0
Tested locally on Clang 16


@Kalixio Kalixio changed the title Fix constexpr on clang Fix constexpr on clang for eastl Sep 1, 2023
@Kalixio Kalixio mentioned this pull request Sep 1, 2023
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@AbrilRBS
Copy link
Member

AbrilRBS commented Sep 4, 2023

Hi @Kalixio thanks a lot for your contribution. Given that this is a patch with uncertain implications, I think it'd be better if a PR was submitted upstream for this code. Once (if) it gets merged, as EASTL's releases are sporadic at best, we can backport the fix.

Can you open the PR in EA's repo and link it here? I'm sure they'll appreciate it :)

@Kalixio
Copy link
Contributor Author

Kalixio commented Sep 5, 2023

We applied this patch in #3643 because they updated the upstream to not use this code anymore and only rely on EA_COMPILER_CPP14_ENABLED (https://github.com/electronicarts/EASTL/blob/05f4b4aef33f2f3ded08f19fa97f5a27ff35ff9f/include/EASTL/internal/config.h#L146), which broke constexpr on msvc 14

We reverted that change with this patch and notified the upstream of the issue (electronicarts/EASTL#402) but no changes or update since then.

Genuinely curious: since we're the ones that created and applied this patch to revert the changes in the upstream, shouldn't we maintain it as well ? (although I understand this is not ideal...)

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 4 (9dc731af062b89ed5031064c7f0dcbcdc01451d1):

  • eastl/3.21.12:
    All packages built successfully! (All logs)

  • eastl/3.18.00:
    All packages built successfully! (All logs)

  • eastl/3.16.07:
    All packages built successfully! (All logs)

  • eastl/3.16.05:
    All packages built successfully! (All logs)

  • eastl/3.17.06:
    All packages built successfully! (All logs)

  • eastl/3.17.03:
    All packages built successfully! (All logs)

  • eastl/3.16.01:
    All packages built successfully! (All logs)

  • eastl/3.15.00:
    All packages built successfully! (All logs)


Conan v2 pipeline ❌

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping @conan-io/barbarians on the PR and we will help you.

See details:

Sorry, the build is only launched for Access Request users. You can request access writing in this issue.

@stale
Copy link

stale bot commented Oct 15, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 15, 2023
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Romain BOULLARD seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions bot removed the stale label Jun 20, 2024
Copy link
Contributor

github-actions bot commented Aug 8, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Aug 8, 2024
@Kalixio Kalixio closed this by deleting the head repository Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants