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

Portmidi header change breaks old code #72

Open
Starbuck5 opened this issue May 20, 2024 · 10 comments
Open

Portmidi header change breaks old code #72

Starbuck5 opened this issue May 20, 2024 · 10 comments

Comments

@Starbuck5
Copy link

Hello, I work on maintaining pygame-ce, which uses portmidi for the pygame.midi module

We've recently seen compiles start to fail in our autogenerated pypm.c

src_c/pypm.c:7236:81: error: incompatible function pointer types passing 'PtTimestamp (*)(void)' (aka 'int (*)(void)') to parameter of type 'PmTimeProcPtr' (aka 'int (*)(void *)') [-Wincompatible-function-pointer-types]
 7236 |   __pyx_v_err = Pm_OpenInput((&__pyx_v_self->midi), __pyx_t_2, NULL, __pyx_t_3, (&Pt_Time), NULL);
      |                                                                                 ^~~~~~~~~~
/data/data/com.termux/files/usr/include/portmidi.h:399:31: note: passing argument to parameter 'time_proc' here
  399 |                 PmTimeProcPtr time_proc,
      |                               ^
1 error generated.

From my research, it looks like the function Pt_Time used to be compatible with PmTimeProcPtr, but now it is not.

I think this broke here: 64314cc#diff-a92ece8a1b564c101a59ed27031b813d53906570f99c4f12bd09ac5f3b65fd82L79-R84

Apparently, in C, a function declaration with no args does not mean the function has no args-- https://stackoverflow.com/a/5929736/13816541. So Pt_Time() and Pt_Time(void) are different.

We have a PR to insert a shim to make Pt_Time used to be compatible with PmTimeProcPtr on any version of portmidi, but I wanted to send in this bug report as an FYI.

@rbdannenberg
Copy link
Contributor

I don't see the PR - maybe that was to fix pypm.c? One way to handle this is to cast Pt_Time to PmTimeProcPtr if you want to use it as a PmTimeProcPtr. Pt_Time does not require a parameter. I'm not too clear on whether this is still correct C or it just works (it was certainly correct on earlier C compilers). But, it's certainly not the cleanest approach. Changing the type signature of Pt_Time is really an incompatible change, but I think it's the right thing to do. It means any direct call to Pt_Time should change to Pt_Time(NULL).

@Starbuck5
Copy link
Author

Here's the PR on our end if you're curious about our fix: pygame-community/pygame-ce#2863

One way to handle this is to cast Pt_Time to PmTimeProcPtr if you want to use it as a PmTimeProcPtr

This was my first instinct too, but I couldn't figure out how to generate that C code out of Cython (the Python-esque language that generates into pypm.c)

@oddbookworm
Copy link

oddbookworm commented May 21, 2024

We still get an error when trying to case Pt_Time to PmTimeProcPtr because the typedef of PmTimeProcPtr says it needs a void* argument

https://github.com/PortMidi/portmidi/blob/master/pm_common/portmidi.h#L339

@rbdannenberg
Copy link
Contributor

You tried (PmTimeProcPtr) &Pt_Time?

@oddbookworm
Copy link

Yes, here's exactly what the code looks like:

        err = Pm_OpenInput(&(self.midi), input_device, NULL, buffersize,
                           <PmTimeProcPtr>&Pt_Time, NULL)

And the compiler error

      Error compiling Cython file:
      ------------------------------------------------------------
      ...
              cdef PmError err
              self.device = input_device
              self.debug = 0

              err = Pm_OpenInput(&(self.midi), input_device, NULL, buffersize,
                                 <PmTimeProcPtr>&Pt_Time, NULL)
                                 ^
      ------------------------------------------------------------

      C:\Users\andre\Projects\pygame-ce\src_c\cython\pygame\pypm.pyx:545:27: Cannot assign type 'PmTimeProcPtr' (alias of 'PmTimestamp (*)(void *) noexcept') to 'long (*)(void) noexcept'

@rbdannenberg
Copy link
Contributor

This is not C or C++. I guess you are saying the solution for C programs: (PmTimeProcPtr) &PtTime does not have an equivalent in Cython.

@oddbookworm
Copy link

That is the Cython equivalent, the <PmTimeProcPtr>&Pt_Time

@oddbookworm
Copy link

I'll try to manually compile the cython without the cast, then do the C-style cast in the C manually to see what happens

@oddbookworm
Copy link

Ok, yeah, manually casting it in the generated C does fix the problem. I've opened an issue with cython to report a potential bug in casting here

@rbdannenberg
Copy link
Contributor

Thanks for doing the test. Although it does sound like cython should work differently, I now also think Pt_Time should match PmTimeProcPtr and I should change all PmTimeProcPtr to PtTimeProcPtr so that the type of the time function will be declared in porttime.h. That will eliminate some casts, enable more type checking, but probably break something else. It shouldn't affect any binaries though.

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

No branches or pull requests

3 participants