libelektra icon indicating copy to clipboard operation
libelektra copied to clipboard

Require C99 and update kdbtypes.h for 1.0

Open kodebach opened this issue 6 years ago • 10 comments

I think the 1.0 release would be a good opportunity to require C99 as the minimum C version.

This would also allow as to use stdint.h types in kdbtypes.h, which would simplify the file a lot. In fact, it might make sense to publicly define the kdb_*_t types as aliases for stdint.h types. Then applications could safely use e.g. int32_t instead of kdb_long_t without worrying about compatibility.

Another advantage of requiring C99+ is that we can use stdbool.h everywhere and actually declare functions like keyIsSystem as bool. This means returning false for NULL pointers, but that is actually better than using int and returning -1, since -1 is a true value in C and therefore if (keyIsSystem (NULL)) has unexpected results.

kodebach avatar Oct 25 '19 11:10 kodebach

C99 is already the minimum C version when compiling Elektra.

This would also allow as to use stdint.h types in kdbtypes.h, which would simplify the file a lot. In fact, it might make sense to publicly define the kdb_*_t types as aliases for stdint.h types. Then applications could safely use e.g. int32_t instead of kdb_long_t without worrying about compatibility.

Yes, I agree that applications should have the guarantee of this mapping. But don't they already have this guarantee if they compile with C99?

The fallback code without this guarantee should only be relevant for applications compiling with C89. It is clearly separated in kdbtypes.h, so yes the file would get shorter but not simpler?

Another advantage of requiring C99+ is that we can use stdbool.h everywhere and actually declare functions like keyIsSystem as bool. This means returning false for NULL pointers, but that is actually better than using int and returning -1, since -1 is a true value in C and therefore if (keyIsSystem (NULL)) has unexpected results.

We will remove keyIsSystem anyway, so this will not be needed, see #3049

markus2330 avatar Oct 26 '19 18:10 markus2330

C99 is already the minimum C version when compiling Elektra.

It is not stated in COMPILE.md. But you are write compiling with older versions doesn't work (at least not with gcc).

But don't they already have this guarantee if they compile with C99?

They do, but we don't state it anywhere. So I wouldn't really call it a guarantee.

It is clearly separated in kdbtypes.h, so yes the file would get shorter but not simpler?

The file would get shorter. It would be simpler to read IMO since the C99 stuff is more or less just typedefs and also AFAIK the file could be static and not require CMake's configure_file, if we ignore pre C99 versions.

We will remove keyIsSystem anyway,

Well, keyIsSystem was just an example... There are also other boolean-type functions e.g. keyNeedSync or keyIsBelow.

kodebach avatar Oct 26 '19 21:10 kodebach

It is not stated in COMPILE.md. But you are write compiling with older versions doesn't work (at least not with gcc).

Good point. I updated this in 07693378b

They do, but we don't state it anywhere. So I wouldn't really call it a guarantee.

Good point, we are somehow missing a tutorial on how to use types in Elektra. I added it in #280

The file would get shorter. It would be simpler to read IMO since the C99 stuff is more or less just typedefs and also AFAIK the file could be static and not require CMake's configure_file, if we ignore pre C99 versions.

Yes, I agree. But now you added/fixed it, so why remove it? Do you think it might not be working in some cases?

There are also other boolean-type functions e.g. keyNeedSync or keyIsBelow.

It would be possible that we use _Bool in our interface and users of our API decide themselves if they want to include stdbool.h, which might be a big problem for larger code bases. Nevertheless, keyIsBelow is very frequently used, so it would be a lot of work. The benefits in C are very small. So it is a very low-priority issue.

So in total: yes, when we would design a new API directly using C99 types would have been the way to go. As we already have the working ifdefs, I would leave it as-is as there are many other more important issues. Important is to have a tutorial explaining how to use types between different languages. In this tutorial we can presume C99 and directly use C99 types (as in the high-level API).

Things like examples/highlevel/pkgconfig/application.c: const int myint = elektraGetLong (elektra, "myint");we should remove, however. Either C99 fixed-size types or Elektra types.

markus2330 avatar Oct 27 '19 09:10 markus2330

It would be possible that we use _Bool in our interface and users of our API decide themselves if they want to include stdbool.h, which might be a big problem for larger code bases. Nevertheless, keyIsBelow is very frequently used, so it would be a lot of work. The benefits in C are very small. So it is a very low-priority issue.

The return type isn't that important actually. The main point is that functions like keyIsBelow should only return 0 or 1 but never -1 to make it harder to use it wrongly in ifs. A boolean return type just makes it more obvious that this should be the case.

kodebach avatar Oct 27 '19 13:10 kodebach

Ok, so I moved the discussion about the return value to #3123 as it is unrelated to C99 and kdbtypes.h.

@haraldg @beku @machinekoder do you have opinions about if it is okay that we require applications using Elektra to use at least C99? (for the high-level API we already do, for the low-level API currently not)

markus2330 avatar Oct 28 '19 08:10 markus2330

Yes, using C99 is definitely okay from my POV.

Actually, I'd even prefer it, if it gives us data types, that I recognize on first sight instead of having to look them up somewhere.

haraldg avatar Oct 28 '19 13:10 haraldg

Ok, seems like nobody is against it. But please handle it as low-priority issue as better docu should already fix the problem (for C99 users there will not be any difference anyway).

markus2330 avatar Oct 31 '19 13:10 markus2330

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

stale[bot] avatar Mar 17 '22 23:03 stale[bot]

ping

kodebach avatar Mar 17 '22 23:03 kodebach

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

github-actions[bot] avatar Mar 18 '23 01:03 github-actions[bot]

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

github-actions[bot] avatar Mar 18 '24 01:03 github-actions[bot]

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

github-actions[bot] avatar Apr 01 '24 01:04 github-actions[bot]