Fix a crash when named_captures called on new instance
#61
Conversation
|
CI fails since a new test case for crash was added. |
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
Could you also add a NULL check to the implementation.
The CI job failures show that installed strscan gem isn't used (Ruby's strscan is used)...
ext/strscan/strscan.c
Outdated
| data.self = self; | ||
| data.captures = rb_hash_new(); | ||
| onig_foreach_name(RREGEXP_PTR(p->regex), named_captures_iter, &data); | ||
| if (p->regex != Qnil) { |
There was a problem hiding this comment.
| if (p->regex != Qnil) { | |
| if (!RB_NIL_P(p->regex)) { |
okuramasafumi
force-pushed
the
fix-named_capture-crash
branch
from
f32ab68 to
187aea6
Compare
|
@kou Thank you, I've fixed it. |
Oh, sorry. I wanted to say "Could you also add a
It's better but it's not related to the CI failures. |
```ruby
StringScanner.new('foo').named_captures
```
This commit fixes a crash in the case above.
okuramasafumi
force-pushed
the
fix-named_capture-crash
branch
2 times, most recently
from
3f79064 to
8bc404c
Compare
|
@kou OK, so I changed JRuby version as well and |
run-test.rb
Outdated
| @@ -1,5 +1,6 @@ | |||
| #!/usr/bin/env ruby | |||
|
|
|||
| require "bundler/setup" | |||
There was a problem hiding this comment.
If we add this, we always use local strscan not strscan gem.
https://github.com/ruby/strscan/actions/runs/4160453753/jobs/7197443977#step:11:5
Loaded strscan from /home/runner/work/strscan/strscan/lib/strscan.so
Gem from /home/runner/work/strscan/strscan
We should use strscan gem like https://github.com/ruby/strscan/actions/runs/4134931150/jobs/7146703286#step:11:5 :
Loaded strscan from /opt/hostedtoolcache/Ruby/3.1.3/x64/lib/ruby/gems/3.1.0/gems/strscan-3.0.7/lib/strscan.so
Gem from /opt/hostedtoolcache/Ruby/3.1.3/x64/lib/ruby/gems/3.1.0/gems/strscan-3.0.7
There was a problem hiding this comment.
I see, I reverted that change with force push.
Should I do something to make CI pass? I believe after releasing a new gem including this fix Ci will become green.
Or more general question, what should we do if a fix and a test are included in the same commit?
There was a problem hiding this comment.
Should I do something to make CI pass?
Can you take a look at why installed gem isn't used only with Ruby 3.2 and master?
Or could you create an issue for this?
It may be a RubyGems/Bundler bug in Ruby 3.2 and master...
I believe after releasing a new gem including this fix Ci will become green.
It's not the intention for the test. The test should use "not-released" gem to prevent releasing a buggy gem.
Or more general question, what should we do if a fix and a test are included in the same commit?
If our CI jobs work as expected (installed gem is used), our test will be passed in the case.
There was a problem hiding this comment.
OK so this might be an issue outside of strscan, now I understand.
I'll try to resolve it myself, and if it's too difficult I'll submit an issue.
okuramasafumi
force-pushed
the
fix-named_capture-crash
branch
from
8bc404c to
36447b8
Compare
|
The output from
It looks like |
okuramasafumi
force-pushed
the
fix-named_capture-crash
branch
from
278a1ac to
36447b8
Compare
|
What do we need to do to merge this? I ran in to the same bug. 😅 |
|
We need to debug a problem that Ruby 3.2 and Ruby master doesn't use |
|
FYI: I fixed installation failure related C-ext default gems ruby/rubygems#6430. I'm not sure this fix resolve issue of this PR. |
|
Oh, thanks for the info. |
|
I merge this for now. |
How was this fixed? I don't see any change in https://github.com/ruby/strscan/commits/master/.github/workflows |
5 participants
This commit fixes a crash in the case above.