Skip to content

Conversation

trufae
Copy link
Contributor

@trufae trufae commented Nov 22, 2024

No description provided.

@trufae trufae mentioned this pull request Nov 22, 2024
3 tasks
@bnoordhuis
Copy link
Contributor

Why? It's a c11 project.

@trufae
Copy link
Contributor Author

trufae commented Nov 22, 2024

It’s the only variable defined inside the for statment and pops a warning when compiling it from other projects that dont set c99 compat. I would appreciate it

@saghul
Copy link
Contributor

saghul commented Nov 23, 2024

Yes it's the only change... for now.

I don't mind landing the change, but I worry we'll have to find ourselves defending the inclusion of c11 features as a requirement in the future, when this project has set c11 as the requirement from the start.

Why can't you compile QuickJS in c11 mode?

@trufae
Copy link
Contributor Author

trufae commented Nov 23, 2024

R2 target build support for old compilers, i’m gradually deprecating some and i prefer not to add extra flags when compiling C code. For example visual studio only added support for c11 in 2022 iirc.. and r2 builds on windows xp and other ancient unixes like pre-2000 AIX systems that are still running nowadays. I would like to have workarounds for portability reasons if needed. But considering this is just the only thing that compilers complain it shouldnt really harm. I can have local patches if needed. But i totally understand the qjsng requirements, and personally from the coding style pov, definining variables inside the for statement always looked a like a c++ism to me. But anyway thats just personal needs that wanted to share, whatever you choose to do with the pr im fine :)

@bnoordhuis
Copy link
Contributor

I'm okay with landing this but it's at best temporary reprieve. I'm curious, what will you do when we merge something old compilers simply choke on?

@trufae
Copy link
Contributor Author

trufae commented Nov 23, 2024

That’s a problem for my future self (:

@saghul saghul merged commit df44d66 into quickjs-ng:master Nov 24, 2024
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