hpyproject / hpy

HPy: a better API for Python
https://hpyproject.org
MIT License
1.02k stars 52 forks source link

Use fixed-width integers and clarify encoding of C strings. #394

Closed fangerer closed 1 year ago

fangerer commented 1 year ago

Resolves #343 and #355 .

As we decided in the last HPy dev call, we want to use fixed-width integers (where appropriate) instead of int/long in the API/ABI because the sizes of int/long could vary depending on several properties.

Since int is actually mostly used as a flag, status code, or small enum values (like in HPy_RichCompare argument int op), this PR is mainly about HPyLong_From(Long|LongLong).

I did some research about the guarantees and found this: https://en.cppreference.com/w/c/language/arithmetic_types

Based on this document, I assume that int has at least 2 bytes, long at least 4 bytes, and long long at least 8 bytes. I came to the conclusion that instead of long and long long, I'll use int32_t and int64_t because a smaller type could still be beneficial in some cases.

I then implemented the previous APIs like HPyLong_FromLong as inline helpers delegating to HPyLong_From(Int32|Int64) depending on sizeof(long). Since the size of a type is compile-time constant, the dispatching overhead should be zero. In case of CPython, HPyLong_From(Int32|Int64) again delegates to PyLong_From(Long|LongLong) depending on the type sizes (again, compile-time constant).

The second change in this PR makes the encoding of C strings in the HPy API clear. We use UTF-8 everywhere. I checked if the const char * arguments are then really consumed by some UTF-8 conversion which is in most cases HPyUnicode_FromString.

mattip commented 1 year ago

On what systems are the macros #if SIZEOF_LONG >= SIZEOF_INT32 and #if SIZEOF_LONG_LONG >= SIZEOF_INT64 true? Or in other words, where will CPython go via the slower c-api calls?

fangerer commented 1 year ago

On what systems are the macros #if SIZEOF_LONG >= SIZEOF_INT32 and #if SIZEOF_LONG_LONG >= SIZEOF_INT64 true? Or in other words, where will CPython go via the slower c-api calls?

You mean: on what system are conditions SIZEOF_LONG >= SIZEOF_INT32 and SIZEOF_LONG_LONG >= SIZEOF_INT64 false ? Because they are true on, e.g. my system since sizeof(long) == sizeof(long long) == 8 these days.

TBH, I don't know. I just tried to cover all possible cases according to the minimal sizes guaranteed by https://en.cppreference.com/w/c/language/arithmetic_types. The slower C API calls will go to e.g. ctx_Long_FromUInt32_t (in ctx_long.c) and currently, it will also just delegate to PyLong_FromLong but the difference is: if SIZEOF_LONG >= SIZEOF_INT32 is false, then you can't even build this function (see ctx_long.c:13). We could, ofc, also do a range check and that case but I thought that can be future work if we ever need it.

mattip commented 1 year ago

OK, thanks. Even though this is a big renaming, I like it. The names are now much more self-explanatory.

mattip commented 1 year ago

There is a failure in cppcheck, which I think is causing the other CI runs to abort. Maybe we should be using fail-fast: false in all the non-essential CI runs.

fangerer commented 1 year ago

There is a failure in cppcheck, which I think is causing the other CI runs to abort. Maybe we should be using fail-fast: false in all the non-essential CI runs.

@mattip: I just don't understand why cppcheck is failing. I tried to reproduce locally but it doesn't print me any useful message. There are a lot of messages with level information and then there is a message that not all includes could be resolved. Running with --check-config (as suggested by cppcheck) doesn't give any useful output. I already played around and added include paths and such but nothing helped. Do you know how to debug that?

mattip commented 1 year ago

I just don't understand why cppcheck is failing

I have no idea. I suggest that we do not try to debug it, rather we make passing cppcheck optional since it is flaky and difficult to control. It is meant to help us, but turns out obstructing progress.

thedrow commented 1 year ago

I just don't understand why cppcheck is failing

I have no idea. I suggest that we do not try to debug it, rather we make passing cppcheck optional since it is flaky and difficult to control. It is meant to help us, but turns out obstructing progress.

395 attempts to fix the CPPCheck failures.

We can disable it if it's not useful but it did catch a few bugs related to varargs before so I think it is somewhat useful at the very least.

fangerer commented 1 year ago

395 attempts to fix the CPPCheck failures.

Well, that's my PR where I made it pass on my local box but it still fails in the GitHub CI. Hence, #395 is not (yet) the solution. I'll do some more investigations ...

mattip commented 1 year ago

I would strongly suggest not spending the very limited time we have on the cppcheck CI run. Let it fail, and let the rest of CI continue. The problem right now is that failing cppcheck run stops the rest of the CI, which then means we have to fix cppcheck before we can do anything else. By adding fail-fast: false to all the non-essential checks, we can make faster progress.

fangerer commented 1 year ago

Agreed. I'll do that.

mattip commented 1 year ago

Thanks, now CI runs to completion. The failures do not seem to be related to this PR. Are they failing elsewhere as well?

fangerer commented 1 year ago

Thanks, now CI runs to completion. The failures do not seem to be related to this PR. Are they failing elsewhere as well?

Yes, in #392 . I've seen those problems on my local machine (Mac OS 12) as well. I thought that the GitHub upgrade to Mac OS 12 is happening in March. I already started to look into the problem. I probably pin to Mac OS 11 for now.