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

[Core] Fix more C99 compliance issues #118

Merged
merged 7 commits into from
Dec 30, 2024

Conversation

FintasticMan
Copy link
Contributor

@FintasticMan FintasticMan commented Dec 28, 2024

This fixes almost all the C99 compliance issues in the implementation of Clay.

Now the only issues remaining are:

  • ##__VA_ARGS__ (and related), which isn't easy to solve without switching to C23.
  • Defining a struct within offsetof to get the alignment of a type, which also isn't easy to solve without switching to C11. (Solved using macro magic)
  • A number of times that signed integers are compared with unsigned integers, which will need some rethinking of all the types for lengths and indices etc.
  • Some strangeness with the CLAY_SIZING_FIT and CLAY_SIZING_GROW macros, which don't compile without specifying -std=gnu99 rather than -std=c99 for some reason.
  • (The compiler warning on GCC discussed below. Not actually standards-non-compliant, but it does complain.)

@Mathys-Gasnier
Copy link

Would you mind also fixing MSVC C99 compliance issues in your PR ?
ATM it forces the rust bindings to use a .cpp file for compilation of clay under windows.
The main issue is https://github.com/nicbarker/clay/blob/main/clay.h#L124, a potential fix was provided by emoon:

#ifdef _MSC_VER
    #define CLAY_PACKED_ENUM __pragma(pack(push, 1)) enum __pragma(pack(pop))
#else
    #define CLAY_PACKED_ENUM enum __attribute__((__packed__))
#endif

@FintasticMan
Copy link
Contributor Author

I really can't figure out why the GCC test is failing :-(. I can reproduce it, but I can't figure out which of the changes in this PR are causing it.

@Mathys-Gasnier
Copy link

From what i'm reading, it seems like CLAY__CONFIG_WRAPPER should have braces somewhere around an initializer

@FintasticMan
Copy link
Contributor Author

I've figured out what the issue is, but unfortunately it's not easy to fix... I'll have a think and come back on this soon.

@Mathys-Gasnier
Copy link

Would you mind expanding on what the issue is ?
Also is it for a specific reason you keep some CLAY_INIT calls, and remove others ?

@FintasticMan FintasticMan force-pushed the standards-compliance-impl branch from 30f2877 to 3e8688e Compare December 28, 2024 23:28
This fixes almost all the C99 compliance issues in the implementation of
Clay.

Now the only issues remaining are:
- ##__VA_ARGS__, which isn't easy to solve without switching to C23.
- Defining a struct within offsetof to get the alignment of a type,
  which also isn't easy to solve without switching to C11.
- A number of times that signed integers are compared with unsigned
  integers, which will need some rethinking of all the types for lengths
  and indices etc.
- Some strangeness with the CLAY_SIZING_FIT and CLAY_SIZING_GROW macros,
  which don't compile without specifying -std=gnu99 rather than -std=c99
  for some reason.
To do this, we create a new struct for every new type Clay creates,
which contains the char and the type, to get the offset.
@FintasticMan FintasticMan force-pushed the standards-compliance-impl branch from 3e8688e to 1a08598 Compare December 29, 2024 00:08
GCC throws this warning. It's not an issue, but fixing it properly will
probably need API changes, so *temporarily* just disable the error.
@FintasticMan
Copy link
Contributor Author

Would you mind expanding on what the issue is ? Also is it for a specific reason you keep some CLAY_INIT calls, and remove others ?

It's to do with the fact that we're now initialising the structs with {0} rather than {}, as {} is only added in C23. However, it does mean that the 0 could be initialising a field in a subobject rather than directly in the struct. GCC wants us to add braces around the 0 in that case to make it clear to the programmer that it's initialising a subobject, but that is difficult to do if you want to do it generically for all structs.

My general idea with removing the CLAY__INIT calls is to reduce the number of casts as much as possible. When initialising a struct, it's unnecessary to cast the braces to the right type, as it's already given in the variable declaration. Adding these casts mostly just adds possibilities for type errors to occur, because casts often implicitly tell compilers to not warn about type errors. The only CLAY__INIT calls I've left in are those that are necessary, like when you're creating a struct in the arguments to a function.

@nicbarker
Copy link
Owner

Confirming for posterity that we're going to move forward assuming a compiler that has support for ##__VA_ARGS__ 🙂

@nicbarker
Copy link
Owner

A number of times that signed integers are compared with unsigned integers, which will need some rethinking of all the types for lengths and indices etc.

We will tackle this in a separate PR so we can get this over the line!

@nicbarker nicbarker merged commit c13eef1 into nicbarker:main Dec 30, 2024
3 checks passed
@FintasticMan FintasticMan deleted the standards-compliance-impl branch December 30, 2024 00:13
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