Repository navigation
Use int32_t where Python passes an int32 array - #65
Merged
Merged
Conversation
Fixes ZhuangLab#27, which asked for int32_t instead of int "in many places". The places that matter are fewer than the phrasing suggests, and they are identifiable rather than a matter of taste. C to C calls do not care what sizeof(int) is, since the compiler is self-consistent, and a ctypes.c_int scalar matches a C int at any size. The assumption only bites where Python hands C a numpy int32 buffer and C reinterprets it as int[], because ctypes checks the dtype on the Python side only. A mismatch there is silent memory corruption, not an error. Scanning every *_c.py argtypes block against the C declarations, Python passes an int32 array to 24 functions. 14 already used int32_t, all of them in multi_fit.c and ia_utilities.c: the core was converted at some point and the rest was not, which is why the tree currently holds two conventions with no way to tell which is intended. This converts the remaining ten: frc/frc.c calc_frc frc_counts dbscan/dbscan.c dbscan db_cat, db_l locClSize cl_counts, cl_id recategorize cl_id, category L1H/homotopy_general.c getVisited vis L1H/homotopy_imagea_common.c getPeaks peak_c sa_library/grid.c grid2D grid, i_x, i_y grid3D grid, i_x, i_y, i_z sa_library/cs_decon_utilities.c label labels moments labels and adds stdint.h to the five files that lacked it, plus to homotopy_imagea_common.h, which now names int32_t in a declaration and is included by homotopy_imagea.c, which has no stdint.h of its own. None of the ten has a caller in C, so each change is a declaration and a definition with no ripple. The other 120 int* parameters in the tree never cross the ctypes boundary and are left alone. Note that there are two functions named getPeaks(). The one taking an int32 array is in L1H. decon_storm/mlem_sparse.c has another, whose int *n is an output count passed as byref(c_int()) and bound as c_void_p, so it is not a boundary case, and that file is not in SConstruct in any event. Nothing changes today, and provably so. int32_t is a typedef for int on every platform this package supports, LP64 and LLP64 alike, so compiling each file before and after gives byte identical objects: frc c32457ae2ebeeb8305c9cad368b656f1 dbscan f9900f35d6a712b50692d9b691d23862 homotopy_general decba0bc02496737904edeaa9e1a2c94 homotopy_imagea_common 6c85ec236b7ba856965b41a0c70f9f0e grid 01c7953b01aa98ad064b62127663a2c6 cs_decon_utilities 3cdb3093f179c5fe6f0aaeb92220d69d Both versions have to be compiled from the same filename for that to mean anything, since gcc embeds the source name. No new warnings; the two fread warnings in homotopy_imagea_common.c are pre-existing and appear identically before and after. That identity is also why the compiler cannot check this work: with int32_t and int the same type, a wrong conversion produces no diagnostic. The check is the scan, and it has to report every C definition of a name rather than the first one it finds, which is what originally hid the L1H getPeaks behind the decon_storm one. Note that dbscan.c has CRLF line endings, so it was edited in binary and its line ending count is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
grid.c declared grid3DZInclusive() and documented it under that name, then defined grid3DInclusize(). So the same function was declared without a definition and defined without a declaration at once, which is the pair of defects 413d2c1 and e9ffba5 fixed in multi_fit.h, here in a single function. Nothing calls it, in C or through ctypes. Two of the three occurrences say grid3DZInclusive, and Inclusize is not a word, so the definition is what was wrong. Wrong since 6a48601, the commit that put the file in its current location. Also converts its four array parameters to int32_t, matching grid2D() and grid3D() next to it after 76080b6a. Python does not call this one so it is not a boundary function today, but it takes the same arguments as its siblings, and leaving one function in the file on the old convention would recreate in miniature the split that commit set out to remove. This one cannot be verified by a byte identical object, since renaming a function changes its symbol. Instead, compiling before and after gives a symbol table differing in exactly one line, < 0000000000000160 T grid3DInclusize > 0000000000000160 T grid3DZInclusive at the same address, and a disassembly that is identical instruction for instruction once that one name is normalized. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #27.
The issue asked for
int32_tinstead ofint"in many places". The placesthat matter turn out to be identifiable rather than a matter of taste, and
there are ten of them, not the 130 that a raw count of
int *suggests.C-to-C calls do not care what
sizeof(int)is — the compiler isself-consistent — and a
ctypes.c_intscalar matches a Cintat any size.The assumption only bites where Python hands C a numpy
int32buffer and Creinterprets it as
int[], becausectypeschecks the dtype on the Pythonside only. A mismatch there is silent memory corruption, not an error.
Scanning every
*_c.pyargtypes block against the C declarations, Pythonpasses an
int32array to 24 functions. 14 already usedint32_t, all ofthem in
multi_fit.candia_utilities.c— the core was converted at somepoint and the rest was not, which is why the tree currently holds two
conventions with no way to tell which is intended.
19199451converts the remaining tenfrc/frc.ccalc_frcfrc_countsdbscan/dbscan.cdbscandb_cat,db_llocClSizecl_counts,cl_idrecategorizecl_id,categoryL1H/homotopy_general.cgetVisitedvisL1H/homotopy_imagea_common.cgetPeakspeak_csa_library/grid.cgrid2Dgrid,i_x,i_ygrid3Dgrid,i_x,i_y,i_zsa_library/cs_decon_utilities.clabellabelsmomentslabelsNone has a caller in C, so each change is a declaration and a definition with
no ripple. The other 120
int *parameters never cross the ctypes boundaryand are left alone.
Nothing changes today, and provably so.
int32_tis a typedef forinton every platform this package supports, LP64 and LLP64 alike, so compiling
each file before and after gives byte-identical objects — six checksums are
in the commit message. That identity is also why the compiler cannot check
this work: with
int32_tandintthe same type, a wrong conversionproduces no diagnostic. The check is the scan.
Note there are two functions named
getPeaks. The one taking anint32array is in L1H.
decon_storm/mlem_sparse.chas another whoseint *nis anoutput count passed as
byref(c_int()), so it is not a boundary case, andthat file is not in SConstruct in any event. A scan that keys on function
name must report every definition, not the first, or it hides one behind the
other.
849e4fa3fixes a function that matched nothinggrid.cdeclaredgrid3DZInclusive()and documented it under that name,then defined
grid3DInclusize()— declared-without-definition anddefined-without-declaration at once, wrong since
6a486010. Nothing callsit. Renamed to match the declaration and the comment, and given the same
int32_ttreatment as its two siblings so the file does not keep onefunction on the old convention.
Verified by symbol table rather than checksum, since a rename changes the
symbol: exactly one line differs, at the same address, and the disassembly is
identical instruction for instruction once that name is normalized.
Full test suite passes, 278 tests. No new compiler warnings.