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

Enable support for GCC compler v < 4.9 #154

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Conversation

loganek
Copy link
Contributor

@loganek loganek commented Nov 29, 2023

GCCv4.8 and lower doesn't ship with stdatomic implementation (even though they don't define STD_NO_ATOMICS for c11). If the code is compiled with GCCv4.8 and older, we use builtin GCC atomic operations instead.

The patch was initially proposed in quickjs's mailing group: https://www.freelists.org/post/quickjs-devel/PATCH-support-for-older-gcc-versions-whitespace-changes-excluded

@loganek loganek force-pushed the master branch 4 times, most recently from 302b164 to 46cdfda Compare November 29, 2023 05:56
Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Left some comments, LGTM otherwise.

Not sure for how long we'll be able to keep support for such an old GCC going forward, but I guess we can give it a try.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@loganek loganek force-pushed the master branch 7 times, most recently from 7613b7f to 628b961 Compare November 29, 2023 06:36
.github/workflows/ci.yml Outdated Show resolved Hide resolved
GCCv4.8 and lower doesn't ship with stdatomic implementation
(even though they don't define __STD_NO_ATOMICS__ for c11).
If the code is compiled with GCCv4.8 and older, we use builtin
GCC atomic operations instead.

The patch was initially proposed in quickjs's mailing group:
https://www.freelists.org/post/quickjs-devel/PATCH-support-for-older-gcc-versions-whitespace-changes-excluded
@bnoordhuis
Copy link
Contributor

I appreciate you took the time to put together a pull request, and I don't strenuously object to this patch, but I confess I'm lukewarm about adding support for a gcc release line that was last updated 10 years ago. Where in this day and age do you even run into 4.8 anymore?

@saghul
Copy link
Contributor

saghul commented Nov 29, 2023

I remember sharing the same sentiment when the patch was originally sent to the QuickJS mailing list.

The answer seems to be "IoT crap" with frozen in time toolchains.

IMHO if it doesn't get in the way, let's let it in, with a big warning on the wall: if it becomes a problem it goes out. WDYT?

@bnoordhuis
Copy link
Contributor

I'm sorta okay with that, just that I fully expect the lifetime of this patch to be measured in weeks or months, not years.

@saghul saghul merged commit 6997445 into quickjs-ng:master Nov 29, 2023
24 checks passed
@saghul
Copy link
Contributor

saghul commented Nov 29, 2023

Let's give it a try. If anything, we could cut a release just before dropping support, when that happens so those interested can just keep using the old version.

@loganek
Copy link
Contributor Author

loganek commented Nov 29, 2023

Thanks, I do appreciate your concerns. If the patch ever slows the overall development down, feel free to revert it. If at that time my team still needs the change, we'll find a way to maintain it.

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.

3 participants