wlav / CPyCppyy

Other
23 stars 20 forks source link

Improve integer type checking in `Utility::GetBuffer()` #12

Closed guitargeek closed 8 months ago

guitargeek commented 9 months ago

The inconsistent types were also observed on 32-bit Linux.

In ROOT, this was hotfixed by adding a check for 32-bit Linux in the preprecessor check, but this required including RConfig.h. I don't think this is desirable.

A better and more direct check for the underlying problem case is to check if long int and int have the same size.

This is part of the effort to reduce the differences between PyROOT and upstream CPyCppyy.

Related commits:

wlav commented 8 months ago

Yes, removal isn't going to work on 64b platforms where long int is 8 bytes and int is 4 bytes. Note that on Windows, both 32b and 64b, long int and int are 4 bytes. Ie., there are two things happening here: if long int and int are the same size (on Windows and, as you report, on 32b Linux), ctypes isn't too picky about the type format and it doesn't matter. Where it does matter, ctypes is picky and cppyy should be, too.

guitargeek commented 8 months ago

Thank you very much for the explanation! I have updated the PR to explicitly compare the type sizes. Like this, we should fix the problem in a general way, as you suggested in the meeting yesterday.

Let's see if the corresponding ROOT PR is green: https://github.com/root-project/root/pull/14655

guitargeek commented 8 months ago

@wlav, the ROOT CI looks good! Can we merge this? Thanks