Cleanup typemap file-finding code and change priority #23694
Closed
Conversation
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
Contributor
|
I haven't studied the code changes in detail, nor do I have experience in writing typemaps (or XS in general). But I appreciate the clean work embodied in this pull request. Each commit, configures, builds and tests earning a PASS. That means that the commits will never confuse a bisection process. Thanks, Dave! |
tonycoz
approved these changes
Sep 10, 2025
ExtUtils::ParseXS imports standard_typemap_locations() but doesn't actually use it: the code which used to need it has since been moved to ExtUtils/ParseXS/Utilities.pm. So remove it from the import list.
Add some more tests to
ExtUtils-ParseXS/t/101-standard_typemap_locations.
In particular, this time with a faked-up @inc. This is mainly
so that the next few commits will only change the things we expect
when updating standard_typemap_locations().
Note that this new test directly tests for expected strings such as
'../../typemap', rather than using File::Spec->updir() etc. This is to
avoid the test code being essentially a duplicate of the sub code. It
skips this test on platforms which don't seem to use '..' etc.
standard_typemap_locations() returns a set of filenames for where a typemap file map be expected, relative to both the current directory and to @inc. The @inc files are filtered using -e, but the others (../../typemap etc) aren't. This commit removes the -e filtering for @inc, for consistency. This means that standard_typemap_locations() now returns a list of filenames of *all* the standard locations where a typemap file *might* be found, regardless of whether the file exists. Previously it was a weird hybrid. This commit should make no difference in practice, since the one caller of this function, process_typemaps() does it's own -f filtering too. So really the net effect of this commit is to remove a few duplicate stat() OS calls. It also allows one existing test to be simplified, since now there will always be one returned entry per @inc entry, whereas before it might have been less.
Simplify the text describing what this function does.
standard_typemap_locations() has a static variable, @tm_template, which it populates with all the '../../typemap' etc paths on first call, then reuses on subsequent calls. This doesn't seem very useful, since standard_typemap_locations() is typically only called once per run of xsubpp etc. And anyway, regenerating those paths (which doesn't involve any IO) isn't costly. So simplify the code by removing the cache.
Reindent after previous removed some scopes. White-space only.
Currently this function builds a list of paths in reverse order, but *unshifts* each result one by one so that they end up in the right order. This commit switches the order of generation and *pushes* the result each time. The net result is the same, but its less cognitive load working out what's going on.
Make them a bit clearer.
What the man page said concerning typemaps, and especially the search path, was out of date.
Prior to 5.10. / 5.8.9, any -typemap arguments to xsubpp were applied last, and thus had the highest priority. From 5.10.0 this was changed (probably inadvertently) so that the -typemap entries were applied first, and so the system and ./typemap files took priority. This commit reverses that change, so that -typemap files are again applied last.
iabyn
force-pushed
the
davem/typemap_order
branch
from
75f477f to
cd875ae
Compare
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.
Cleanup typemap file-finding code and change priority
All but the last commit in this series clean up, simplify, and better document and test the code which is responsible for searching for XS typemap files in standard locations (relative to both @inc and the current dir). They all (in theory) have no change in behaviour.
The final commit restores the priority of files specified -typemap: these were the highest priority before 5.10.0 and became lowest priority afterwards. Pre-5.10.0 was still the documented behaviour and I think the change was a mistake. In practice this last commit is unlikely to affect any real modules, but you never know.