Skip to content

Remove declarations inside for loops #4140

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

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

emasab
Copy link
Contributor

@emasab emasab commented Jan 9, 2023

No description provided.

@emasab emasab force-pushed the feature/remove-declarations-inside-for-loops branch from b846771 to 94dfefb Compare January 10, 2023 08:31
Copy link
Member

@PrasanthV454 PrasanthV454 left a comment

Choose a reason for hiding this comment

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

Have a small doubt here. Any particular reason behind making this change. Generally I think best practice is to use the smallest scope possible for variables.

@pranavrth
Copy link
Member

Have a small doubt here. Any particular reason behind making this change. Generally I think best practice is to use the smallest scope possible for variables.

This format is not supported in older version of C (before C99).

@emasab
Copy link
Contributor Author

emasab commented Jan 10, 2023

Let's check just declarations in for loops. As for declaration before statements, it's better to avoid them, but it's not a strict requirement as if you compile with -Wdeclaration-after-statement you already get a lot of warnings in many places.

@PrasanthV454
Copy link
Member

Have a small doubt here. Any particular reason behind making this change. Generally I think best practice is to use the smallest scope possible for variables.

This format is not supported in older version of C (before C99).

Got it. thanks

@Quuxplusone
Copy link
Contributor

I'd recommend not merging this patch. It's true that C89 didn't support declarations in for loops, but it's also been 34 years since 1989, and you have to go really really far out of your way in the year 2023 to convince a compiler to complain about things that have been legal in C for probably the whole lifetime of many of the humans reading this.

Also, as mentioned in #4140 (comment), librdkafka already uses C99-isms in many places. If you took a time machine back to the '80s and tried to compile librdkafka with one of those compilers, you'd find that they complain about a lot more than just variable declarations — they'd complain about // comments, the unsigned long long data type, and so on. Go back another 10 years and you'd find them complaining about the use of the keyword void.

Basically, I think librdkafka should assume the use of a compiler about as old as Kafka itself (i.e. 2011), but going further back in time than that seems impractical and masochistic.

@emasab
Copy link
Contributor Author

emasab commented Jan 11, 2023

@Quuxplusone this is not for being C89 compatible, we're following the project declaration:


This is a mix of C89 and C99, to be compatible with old MSVC versions.

Notable, it is C99 with the following limitations:

 * No variable declarations after statements.
 * No in-line variable declarations.

But, limitation 1 has to be dropped because of #4140 (comment)
and MSVC only implemented variable declarations after statements in 2013. This PR is about limitation 2, but without the first it won't compile anyway on older MSVC. Also Visual Studio 2012 support ended yesterday.

That said, it seems we should move to standard C99

@Quuxplusone
Copy link
Contributor

@emasab: "this is not for being C89 compatible, we're following the project declaration" — Except that, as you say, you're not following the description in the README. The description in the README describes a world that self-evidently is not our reality: #4140 (comment) . So it seems like what you should do, really, is just make the two-line update to the README's description so that it matches the real world. This PR makes reality worse, but (as you say) doesn't even succeed at implementing the dystopia described in the README.

@emasab
Copy link
Contributor Author

emasab commented Jan 13, 2023

@Quuxplusone don't be upset, given my last comment I hoped it was clear that I'm checking if we can remove these statements in the README, and I've still not merged this for that reason.

@emasab
Copy link
Contributor Author

emasab commented Jan 13, 2023

We need to merge this as we support RHEL/CentOS 7, whose EOL is in 2024. CentOS has a GCC version before 5.1.0 where the default was changed from gnu90.

With these fixes it compiles correctly configuring with gnu90, and gives error without, so that's the minimum C "standard" we need to support.

We're going to include this in the README and in the CI. Up to 2024 when CentOS 7 will reach EOL.

@emasab emasab merged commit 6f32d2d into master Jan 13, 2023
@emasab emasab deleted the feature/remove-declarations-inside-for-loops branch January 13, 2023 18:32
milindl pushed a commit that referenced this pull request Feb 10, 2023
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.

4 participants