Conversation
jkeenan
left a comment
jkeenan
left a comment
There was a problem hiding this comment.
Scope of review so far largely limited to lib/ExtUtils/ParseXS/Node.pm and limited to (i) locating places in test suite where warnings and error messages are or are not being exercised; and (ii) spelling.
|
Note: GH web interface is saying "This branch cannot be rebased due to conflicts". However, I created a local branch from the p.r. and was able to rebase it on blead. |
jkeenan
left a comment
jkeenan
left a comment
There was a problem hiding this comment.
If I checkout this commit and attempt to configure/build/test it by itself, I get a failure during the configuration stage.
$ git checkout 195e097f4395c5332a1401201872881cacf7dec8
Note: switching to '195e097f4395c5332a1401201872881cacf7dec8'.
... [skip]
HEAD is now at 195e097f43 ParseXS: rename t/002-more.t to t/302-run-more.t
[perlmonger: perl2] $ configure_and_test
First let's make sure your kit is complete. Checking...
ls: dist/ExtUtils-ParseXS/t/302-more.t: No such file or directory
THIS PACKAGE SEEMS TO BE INCOMPLETE.
You have the option of continuing the configuration process, despite the
distinct possibility that your kit is damaged, by typing 'y'es. If you
do, don't blame me if something goes wrong. I advise you to type 'n'o
and contact the author (https://github.com/Perl/perl5/issues).
Continue? [n] n
ABORTING...
[perlmonger: perl2] $ ls -l dist/ExtUtils-ParseXS/t/302-more.t
ls: dist/ExtUtils-ParseXS/t/302-more.t: No such file or directory
[perlmonger: perl2] $ ack '302-more' .
MANIFEST
4217:dist/ExtUtils-ParseXS/t/302-more.t Extended ExtUtils::ParseXS compile and run
UU/xdg
16:dist/ExtUtils-ParseXS/t/302-more.t
UU/missing
1:ls: dist/ExtUtils-ParseXS/t/302-more.t: No such file or directory
Found during an analysis using Perl5::TestEachCommit. This could potentially cause a problem for Porting/bisect.pl. Can you adjust? Thanks.
Now fixed and rebased. |
|
Could you please keep the |
Now done. |
|
"ParseXS: don't skip death() test" commit message: " the death() method as as it used to exit." |
iabyn
force-pushed
the
davem/xs_fixups
branch
from
7af02c8 to
2a13585
Compare
|
Fixed typos in several commit messages: the one Tony pointed out, plus various misspellings of parseXS; fixed up the bad TODO tests; rebased and pushed. |
|
commit "ParseXS: add more function pointer type tests" commit message
|
iabyn
force-pushed
the
davem/xs_fixups
branch
from
2a13585 to
de984d1
Compare
|
Added three new commits to address the recent points; rebased and pushed. |
|
commit "ParseXS: warn on forbidden OVERLOAD names":
|
|
I've gone through every commit at this point. |
I'll add that to my list of things to do in a later batch. |
|
I'll merge this soon unless anyone intends any further review. |
tonycoz
left a comment
tonycoz
left a comment
There was a problem hiding this comment.
Though right now it fails porting/cmp_version
Please make smaller batches :) I understand the need for the large batches in the big refactors, but I think this one could have been done in smaller chunks. I expect you'll get faster feedback and maybe more people looking at 10 commits than at 107 commits. |
Temporarily undo part of v5.43.7-162-g535b3579df, to make rebasing this branch easier. The changes will be added back at the end of the branch.
Move out of 001-basic.t and into their own test file, the tests which are concerned with things which can appear at file-scope in an XS file (i.e. outside an XSUB), apart from file-scoped keywords. This is just several simple cut and pastes of blocks of tests; no changes have been made to the tests themselves. This commit is part of splitting 001-basic.t into several smaller files, with a more coherent grouping.
Move out of 001-basic.t and into their own test file, the tests which are concerned with keywords which can appear at file-scope in an XS file (i.e. outside an XSUB). This is just several simple cut and pastes of blocks of tests; no changes have been made to the tests themselves. This commit is part of splitting 001-basic.t into several smaller files, with a more coherent grouping.
Move out of 001-basic.t and into their own test file, the tests which are concerned with parsing the declaration of an XSUB (i.e. the first two lines). This is just several simple cut and pastes of blocks of tests; no changes have been made to the tests themselves. This commit is part of splitting 001-basic.t into several smaller files, with a more coherent grouping.
Move out of 001-basic.t and into their own test file, the tests which are concerned with parsing the individual parameters of an XSUB. This is just several simple cut and pastes of blocks of tests; no changes have been made to the tests themselves. This commit is part of splitting 001-basic.t into several smaller files, with a more coherent grouping.
Move out of 001-basic.t and into their own test file, the tests which are concerned with parsing the return type of an XSUB. This is just several simple cut and pastes of blocks of tests; no changes have been made to the tests themselves. This commit is part of splitting 001-basic.t into several smaller files, with a more coherent grouping.
Move out of 001-basic.t and into their own test file, the tests which are concerned with parsing the INPUT and OUTPUT keywords of an XSUB. This is just several simple cut and pastes of blocks of tests; no changes have been made to the tests themselves. This commit is part of splitting 001-basic.t into several smaller files, with a more coherent grouping.
Move out of 001-basic.t and into their own test file, the tests which are concerned with parsing the keywords (except INPUT and OUTPUT) of an XSUB. This is just several simple cut and pastes of blocks of tests; no changes have been made to the tests themselves. This commit is part of splitting 001-basic.t into several smaller files, with a more coherent grouping.
Move out of 001-basic.t and into their own test file, the tests which are concerned with parsing the C++ support features for XSUBs. This is just several simple cut and pastes of blocks of tests; no changes have been made to the tests themselves. This commit is part of splitting 001-basic.t into several smaller files, with a more coherent grouping.
In the previous commits, most of the body of t/001-basic.t has been moved out into separate 0xx-parse-foo.t files. Rename this file now so that it is also a 0xx-parse- file.
Add the standard boilerplate to the top of the file.
This test data file has not been used since v5.15.0-477-g9b58169ac2, but has been hanging around ever since.
This test file has apparently never actually tested anything, and there's already another test file, t/104-map_type.t which actually tests the map_type() method.
This test file doesn't actually test anything. The set_cond() method is fairly trivial; it just returns a short snippet of C code like "items < 1" for use in an XSUB's number-of-args checking, and there's plenty of tests in the 0xx-parse-foo.t test files for that already.
Each of the 1xx-*.t test files run tests on a particular API function. So rename all these files to include a -api- prefix (in the same way that the parsing files are all 0xx-parse-foo.t and the code running are all 3xx-run-foo.t)
Add a boilerplate comment line explaining the purpose of each file.
The 5xx-t-foo.t files all test ExtUtils::Typemaps (as opposed to all the other test files in the t/ directory which test ExtUtils::ParseXS). Rename the filename prefix from -t- to -typemaps- to better signal this fact. 600-t-compat.t is a bit of an odd one - it's also testing ExtUtils::Typemaps and I'm not sure why its 600 rather than 5xx. It's the only 6xx in the directory. I've renamed it to 518-typemaps-compat.t for consistency.
Add a boilerplate comment line or two explaining the purpose of each file.
This directory just contains typemap files to be used by various test files. Make the name give a clearer indication of its purpose.
Fix up a test to work under perl 5.8.9. This regex: qr/\Q...\".../ was meant to match the two literal characters backslash and double-quote, but that only worked correctly from 5.10.0 onwards.
This private helper module had 'strict' enabled, but not warnings.
So enable them, and fix up a problem it reveals:
the destructor for the tied handle was trying to write the buffer
contents even if the buffer was empty. This is harmless, but was now
triggering a warning for some tests which tested for an error condition
that didn't produce any output, e.g.:
../dist/ExtUtils-ParseXS/t/002-parse-file-scope.t ................... 1/?
print() on unopened filehandle FH at .../CountLines.pm line 44 during global destruction.
This is also a fix for running under 5.8.9, whose test harness rather
anti-socially invokes perl with -w. Which is how I got to know about the
warnings in the first place.
A couple of TODO tests were sort of guessing what C code *should* be generated, and were getting it wrong. For the first: - spurious backtick; - no space after if; - 1 arg not 2. For the second: - no space after if; Since that C code wasn't being generated yet (hence the TODO tests), they were happily passing without it being obvious that it was wrong. Spotted by Tony C.
A couple of tests incorrectly had "should die" in their test descriptions, likely due to cut+paste thinkos.
The parser was silently skipping those two valid overload keys. Also add tests for all currently legal overload keys.
Previously, the OVERLOAD keyword just silently ignored any op names which didn't seem syntactically valid. This commit makes it warn (but still skip). Note that the parser doesn't have a list of valid overload names. For forwards compatibility, it just has a set of valid characters, including \w and most of the standard punctuations characters, and rejects anything else.
The s/STATIC/static/ changes in t/XSMore.xs were temporarily reverted at the start of this branch to help with rebasing. Now add them back.
iabyn
force-pushed
the
davem/xs_fixups
branch
from
ee32679 to
d364576
Compare
4 participants
Another round of ExtUtils::ParseXS improvements.
This branch:
Improves error checking and reporting. Several error messages have been improved, more error messages added etc. The new errors are generally backwards-compatible: they report things which would otherwise have generated broken C code and so failed later in a more confusing way.
Test coverage of the warning and error messages is now comprehensive.
Refactors the length(foo) pseudo-parameter implementation, adds more error checks, and makes it so that foo no longer needs to be a type that maps to T_PV; merely that it maps to a type whose INPUT template contains SvPV_nolen($arg) or similar.
Reorganises the test files and infrastructure. The 7000 line 001-basic.t has now been split into several files, some obsolete files have been deleted, and a new naming system for test files has been introduced:
0xx-parse-foo.t just run the parser on XS snippets and see if the generated C code matches expectations
1xx-api-foo.t test ExtUtils::ParseXS API functions
3xx-run-foo.t parse XS files into C files, compile and run them
5xx-typemaps-foo.t tests for ExtUtils::Typemaps