clay icon indicating copy to clipboard operation
clay copied to clipboard

[Core] Fix more C99 compliance issues

Open FintasticMan opened this issue 11 months ago • 6 comments

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.
  • [x] 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.)

FintasticMan avatar Dec 28 '24 11:12 FintasticMan

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

Mathys-Gasnier avatar Dec 28 '24 12:12 Mathys-Gasnier

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.

FintasticMan avatar Dec 28 '24 12:12 FintasticMan

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

Mathys-Gasnier avatar Dec 28 '24 13:12 Mathys-Gasnier

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.

FintasticMan avatar Dec 28 '24 13:12 FintasticMan

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 ?

Mathys-Gasnier avatar Dec 28 '24 14:12 Mathys-Gasnier

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.

FintasticMan avatar Dec 29 '24 00:12 FintasticMan

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

nicbarker avatar Dec 29 '24 23:12 nicbarker

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 avatar Dec 29 '24 23:12 nicbarker