Skip to content
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

Simplify Kernel signals method using Signal values #1901

Closed
wants to merge 1 commit into from

Conversation

jstuder-gh
Copy link
Contributor

The Kernel signals method previously had a lot of logic for determining
signal values on the host system and backends. Now that that logic is
encapsulated into the getsignals ops, this method can be greatly
simplified.

@jstuder-gh
Copy link
Contributor Author

Changed the or operator (||) to a defined-or (//) and re-pushed. In this instance, it probably wouldn't make to much of a difference, but since the intent is to check against type objects and not falsey, this makes it a bit more explicit.

@lizmat
Copy link
Contributor

lizmat commented Mar 2, 2019

I assume this should still be merged, correct?

@jstuder-gh
Copy link
Contributor Author

It's been a long time since I've looked at this, but I believe it should still be okay to merge in. All three backends have getsignals working (although I do have an old PR on NQP repo to bring parity between the signals available on WIN32 between the backends).

I know that at least one module in the ecosystem was calling this method (I don't remember what that module was called though), but I don't see how this would break anything.

@lizmat
Copy link
Contributor

lizmat commented Mar 3, 2019 via email

The Kernel signals method previously had a lot of logic for determining
signal values on the host system and backends. Now that that logic is
encapsulated into the getsignals ops, this method can be greatly
simplified.
@jstuder-gh
Copy link
Contributor Author

I force-pushed a new version of the branch that is exactly the same as the old version except:

  • Rebased on top of current Rakudo master
  • Replaced hard-coded 32 with value based off Signal.enums.values.max

@JJ
Copy link
Collaborator

JJ commented May 4, 2020

There's a conflict after the change of name. Can you please check?

@AlexDaniel AlexDaniel closed this in 5644c70 May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants