Changes to make Process::Status compatible with MRI #3270
Conversation
While MRI removes the `new` method from Process::Status we _do_ have this method (and an initialize method) that perform certain actions worth testing. More importantly, this way we can ensure that this class can be initialized from C in a similar fashion to MRI. See #3268 for more information.
This ensures that the class can be initialized in a similar fashion to MRI (mainly from C land) while still keeping it compatible with existing Rubinius code.
MRI stores the status code in an instance variable called "@status" instead of "@ExitStatus". Due to the lack of a proper API to set this variable people instead have to resort to setting it via rb_ivar_set() in C. To ensure compatibility with MRI we sadly also have to name this variable in the same way. Due to the variable being renamed the method "exitstatus" now has to be manually defined, instead of just being an attribute reader. See #3268 for more information.
|
MRI eventually ends up using this code to extract the status of a |
These specs now cover the other arguments of the #initialize method.
| Process::Status.new.should be_an_instance_of(Process::Status) | ||
| end | ||
|
|
||
| it 'initializes Process::Status with a PID' do |
There was a problem hiding this comment.
I have a question regarding these specs. Initially I wrote them as following:
describe 'Process::Status#initialize' do
describe 'with a PID' do
it 'returns the PID when calling #pid' do
Process::Status.new(42).pid.should == 42
end
end
describe 'with a PID and exit status' do
before do
@status = Process::Status.new(42, 1)
end
it 'returns the PID when calling #pid' do
@status.pid.should == 42
end
it 'returns the exit status when calling #exitstatus' do
@status.exitstatus.should == 1
end
end
# ...
endWhile this breaks the specs up more nicely and make sure all attributes are set when combined, I can't help but feel that they're getting a bit verbose. Perhaps shared examples would be a solution to this?
There was a problem hiding this comment.
There's not a lot that would be simpler with shared specs. This type of spec (ie initialize, check initialized) is difficult to simplify. Your approach in the current version of the specs here is fine.
The purpose of the specs is to be correct, clear, consistent, and simple, in that order. A more verbose spec can be all of those at once.
| @termsig = termsig | ||
| @stopsig = stopsig | ||
| end | ||
|
|
||
| def exitstatus | ||
| return @status |
There was a problem hiding this comment.
Style - return not needed?
There was a problem hiding this comment.
Good point, I'm so used to always using an explicit return that it goes automatically.
|
MRI appears to store the termsig and stopsig as high bits in http://pubs.opengroup.org/onlinepubs/9699919799/functions/wait.html http://rxr.whitequark.org/mri/source/process.c#087 087 #ifndef WIFEXITED
088 #define WIFEXITED(w) (((w) & 0xff) == 0)
089 #endif
090 #ifndef WIFSIGNALED
091 #define WIFSIGNALED(w) (((w) & 0x7f) > 0 && (((w) & 0x7f) < 0x7f))
092 #endif
093 #ifndef WIFSTOPPED
094 #define WIFSTOPPED(w) (((w) & 0xff) == 0x7f)
095 #endif
096 #ifndef WEXITSTATUS
097 #define WEXITSTATUS(w) (((w) >> 8) & 0xff)
098 #endif
099 #ifndef WTERMSIG
100 #define WTERMSIG(w) ((w) & 0x7f)
101 #endif
102 #ifndef WSTOPSIG
103 #define WSTOPSIG WEXITSTATUS
104 #endif |
|
Oh - @yorickpeterse interesting about .to_i, it returns all of the bits, whereas the exitstatus method returns only the 8 bit exit. |
Changes to make Process::Status compatible with MRI
See #3268 for the issue that led to these changes. Before we merge this I'd like some feedback on this, in particular abotut the renaming of
@exitstatusto@status. I don't think there's a way around this, but just in case there's something better for this I'm creating a PR instead of pushing this straight to the master branch.