Skip to content

Fix undefined flags in base64 #13448

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

Closed
wants to merge 1 commit into from

Conversation

jorgsowa
Copy link
Contributor

@jorgsowa jorgsowa commented Feb 20, 2024

Part of #5526 for base64 ext.

I'm not sure about failing the test. Looks like it's not related.

Checks for flags:

  • ZEND_INTRIN_AVX2_NATIVE
  • ZEND_INTRIN_AVX2_FUNC_PROTO
  • ZEND_INTRIN_AVX2_FUNC_PTR
  • ZEND_INTRIN_AVX2_RESOLVER
  • ZEND_INTRIN_SSSE3_NATIVE
  • ZEND_INTRIN_SSSE3_RESOLVER
  • ZEND_INTRIN_SSSE3_FUNC_PROTO
  • ZEND_INTRIN_AVX512_FUNC_PROTO
  • ZEND_INTRIN_AVX512_FUNC_PTR
  • ZEND_INTRIN_AVX512_VBMI_FUNC_PROTO
  • ZEND_INTRIN_AVX512_VBMI_FUNC_PTR
  • BASE64_INTRIN_AVX512_VBMI_FUNC_PTR
  • BASE64_INTRIN_AVX512_VBMI_FUNC_PROTO
  • BASE64_INTRIN_AVX512_FUNC_PTR
  • BASE64_INTRIN_AVX512_FUNC_PROTO
  • BASE64_INTRIN_AVX512_VBMI_FUNC_PTR
  • HAVE_FUNC_ATTRIBUTE_TARGET

@jorgsowa jorgsowa requested a review from bukka as a code owner February 20, 2024 23:49
@jorgsowa jorgsowa marked this pull request as draft February 21, 2024 07:50
@jorgsowa jorgsowa marked this pull request as ready for review February 21, 2024 16:36
@jorgsowa jorgsowa force-pushed the fix_undef_flags_in_base64 branch from 4189b06 to f53f80d Compare February 22, 2024 21:50
@jorgsowa jorgsowa closed this May 18, 2024
@petk
Copy link
Member

petk commented May 19, 2024

This would still be good to fix at some point I think. There are a lot of these, yes.

@jorgsowa
Copy link
Contributor Author

There are still many such cases but this required reviews. I assume there are more important things to tackle than this. I don't want to spam with such PRs so I just closed it. It's better probably to merge Girgias's PR at once.

@petk
Copy link
Member

petk commented May 23, 2024

I think it would be better to do it in multiple PRs like this one. There are too many of these for a single PR because some 0/1 value can get quickly missed - I've seen this in the past on few places and it can introduce a bigger bug than before. And BTW, I'm hitting such issues daily with my build. So from my PoV all such fixes are a high priority.

I think all is good here to merge. It only needs to be rebased against the master branch.

@jorgsowa
Copy link
Contributor Author

jorgsowa commented May 23, 2024

I couldn't reopen PR, so I recreated it. If it's a priority issue then I can spend time fixing it.

#14316

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