search

jpype-project / jpype

JPype is cross language bridge to allow Python programs full access to Java class libraries.
http://www.jpype.org
Apache License 2.0
1.12k stars 183 forks source link

prevent misuse of PyTuple_Pack #1218

Closed astrelsky closed 1 month ago

astrelsky commented 2 months ago

This prevents simple mistakes such as the following

https://github.com/jpype-project/jpype/blob/66f8c6c07e051eb430326418798736004b0f833b/native%2Fpython%2Fpyjp_number.cpp#L390

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 97.87234% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.21%. Comparing base (66f8c6c) to head (1b76a40). Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
native/python/pyjp_package.cpp 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1218 +/- ## ========================================== - Coverage 87.21% 87.21% -0.01% ========================================== Files 113 113 Lines 10281 10277 -4 Branches 4065 4088 +23 ========================================== - Hits 8967 8963 -4 Misses 718 718 Partials 596 596 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Thrameos commented 2 months ago

Minor suggestion.... given that all uses of this function should be placed in a JPPyObject wrapper via call to we don't leak it would seem like combining the two would make sense. It would take some minor reworking of a few sections which were already dangerous for exceptions.

astrelsky commented 2 months ago

Minor suggestion.... given that all uses of this function should be placed in a JPPyObject wrapper via call to we don't leak it would seem like combining the two would make sense. It would take some minor reworking of a few sections which were already dangerous for exceptions.

Agreed. I'll do this tonight.

astrelsky commented 2 months ago

The changes were mostly painless. I only had to think about one change which was in PyJPClass_FromSpecWithBases. I left a comment where it should not be used in that function.

Seems like the mac compiler is a bit over picky about constexpr. It's technically not wrong, but the standard is not that strict. It was only added out of habit to begin with. Not sure why it doesn't fail to compile anything using a standard c++ header :joy:.

Thrameos commented 1 month ago

Merge for upcoming release.