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

source cleanup: remove all custom typedef structs #5338

Closed
wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented May 4, 2020

  • Stick to a single unified way to use structs
  • Make checksrc complain on 'typedef struct {'
  • Allow them in tests, public headers and examples

@bagder bagder added the tidy-up label May 4, 2020
@danielgustafsson
Copy link
Member

danielgustafsson commented May 5, 2020 via email

@bagder bagder force-pushed the bagder/remove-typedef-structs branch 8 times, most recently from 209482f to 436130e Compare May 8, 2020 08:28
@bagder bagder marked this pull request as ready for review May 8, 2020 09:49
@bagder bagder changed the title [WIP] source cleanup: remove all custom typedef structs source cleanup: remove all custom typedef structs May 8, 2020
@bagder bagder force-pushed the bagder/remove-typedef-structs branch from 436130e to 10bd507 Compare May 13, 2020 11:26
@bagder
Copy link
Member Author

bagder commented May 13, 2020

Buckle up. I'm going to land this asap. Just pushed a rebase and if things still are fine this will land.

@bagder bagder force-pushed the bagder/remove-typedef-structs branch 2 times, most recently from c4f3ed4 to 6184194 Compare May 13, 2020 22:05
 - Stick to a single unified way to use structs
 - Make checksrc complain on 'typedef struct {'
 - Allow them in tests, public headers and examples

 - Let MD4_CTX, MD5_CTX, and SHA256_CTX typedefs remain as they actually
   typedef different types/structs depending on build conditions.
@bagder bagder force-pushed the bagder/remove-typedef-structs branch from 6184194 to 402b19d Compare May 14, 2020 06:21
@bagder
Copy link
Member Author

bagder commented May 14, 2020

Someone emailed me and asked me about the motivation for this work and here's basically my extended motivation.

Using struct spelled out in code is more clear than typedef'ed structs in the cases where "struct" is present vs not present. With struct present, we know it's a struct. Without it, we don't know what it is (by simply inspecting the name).

But I'm arguing for this change primarily for consistency. Our code has a mix of typedef'ed structs and real structs and there's nothing to hint about when we use either approach and I've come to be annoyed by not knowing. It has basically depended on the mood of the particular author which style that is used in which function.

I want the curl source code to look and feel like it was written by a single author with a single mindset on how code should look like. To make the code clean, easy to read and easy to understand - which then implies consistent. That leads to less bugs and in particular less security problems - a better product. Therefor, I want us to use structs in the code using one consistent way.

@danielgustafsson
Copy link
Member

danielgustafsson commented May 14, 2020 via email

@bagder bagder closed this in 8df4554 May 15, 2020
@bagder bagder deleted the bagder/remove-typedef-structs branch May 15, 2020 06:55
@mkauf
Copy link
Contributor

mkauf commented Jun 2, 2020

Most autobuilds fail, because the ".checksrc" files are not found. The autobuilds perform an "out-of-tree" build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants