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

Add -Wno-implicit-fallthrough flags conditionally #13331

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

petk
Copy link
Member

@petk petk commented Feb 5, 2024

Older GCC versions (< 7.0) don't support the -Wno-implicit-fallthrough compiler flag. This adds the flag conditionally in case some other compiler will run into same issue.

Fixes #13330

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

RHEL 7 is EOL in June of this year.

Does it really make sense to fix this?

Also the main reason for this flag, other than opcache, is because we don't control the bundle libraries. For opcache I don't remember the reason, so it might make sense to remove the flag and see what happens again.

@devnexen
Copy link
Member

devnexen commented Feb 6, 2024

I m a tad on the fence for the change too.

  • mac 10.6 last update was around 2011-ish and gcc 4.2 is very old.
  • The bugtracker mentions php 8.1 they will have to patch this version anyway.
  • but in another hand, the change is not too big so I do not really object. I just hope we won t get more demands in that direction..

@Girgias
Copy link
Member

Girgias commented Feb 6, 2024

I don't mind this change either, but considering that GCC 8.1 supports C17 and RHEL 8 comes with GCC 8.x I would imagine that we might bump our minimum C requirement soon anyway (mainly for the atomics supports I'd gather).

@petk
Copy link
Member Author

petk commented Feb 6, 2024

Hm, these flags are difficult :D I'll wait a bit with this. I've checked some of the compilers and basically all decent compilers today should support this flag. Even Solaris Studio compiler, Intel compiler, and MS Visual Studio with 2019 in its /wd and number form support it. That's good to know. I understand now...

@ryandesign
Copy link
Contributor

I wish you would merge this PR so that I wouldn't have to keep maintaining our patch.

Older GCC versions (< 7.0) don't support the -Wno-implicit-fallthrough
compiler flag. This adds the flag conditionally in case some other
compiler will run into same issue.

Fixes phpGH-13330
@petk petk force-pushed the patch-wno-implicit-fallthrough-flag branch from 8d0b781 to be8edd5 Compare July 22, 2024 04:56
@petk petk merged commit d20d113 into php:PHP-8.2 Jul 22, 2024
6 of 9 checks passed
@petk petk deleted the patch-wno-implicit-fallthrough-flag branch July 22, 2024 04:57
petk added a commit that referenced this pull request Jul 22, 2024
* PHP-8.2:
  Append -Wno-implicit-fallthrough flag conditionally (#13331)
petk added a commit that referenced this pull request Jul 22, 2024
* PHP-8.3:
  Append -Wno-implicit-fallthrough flag conditionally (#13331)
@petk
Copy link
Member Author

petk commented Jul 22, 2024

Sure, let's add this conditionally then. Added to PHP-8.2 and up.

In the upcoming PHP-8.4 this compile flag was even reduced down to the minimum where it is still needed. The ext/date is still pending the merge, so somewhere right after the PHP 8.4 #14187

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.

4 participants