-
Notifications
You must be signed in to change notification settings - Fork 587
Explicitly reject length(NAME) with typemaps other than T_PV #23479
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
base: blead
Are you sure you want to change the base?
Conversation
This one caused my a bunch of grief related to #23150 before I figured out what was going on. |
It needs test(s) for the new error message. Other than that, I approve. |
Currently, in dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS/Node.pm's
Could we write tests for both conditions in this p.r.? |
Previously, the length operator on typemaps other than T_PV would lead to that length value not being initialized, leading to segfaults and worse. Worse yet, parsexs would silently emit this erroneous code. For now it will at least give a clear error, in the future we should perhaps consider eliminating this limitation altogether.
bdf5a3f
to
e214089
Compare
Added that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first part of 001-basic.t uses a crude form of test framework. Newer tests in the file (basically the ones added by me in the last year) use a better framework. It would be better to use this new framework. Look for the "length() no matching var" test; it would be easy and better to add a new test directly after that test.
On Sun, Jul 27, 2025 at 03:31:09AM -0700, James E Keenan wrote:
jkeenan left a comment (Perl/perl5#23479)
Currently, in dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS/Node.pm's `lookup_input_typemap()`, there already exists one error condition at the point where the p.r. proposes to insert a new `die`. That existing `die` itself appears to be unexercised in the test suite.
```
That's because that condition isn't (I think) ever reached. A similar
check+die is done earlier during parsing, and there *is* a test for that
error. I'll add a note to self to remove the redundant check at some
point.
…--
Modern art:
"That's easy, I could have done that!"
"Ah, but you didn't!"
|
Previously, the length operator on typemaps other than T_PV would lead to that length value not being initialized, leading to segfaults and worse. Worse yet, ExtUtils::ParseXS would silently emit this erroneous code.
For now it will at least give a clear error, in the future we should perhaps consider eliminating this limitation altogether.