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

Remove unused JL_HAVE_SIGALTSTACK code #54568

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

fingolfin
Copy link
Contributor

Nothing is ever activating this, and it probably hasn't been tested in a while.

As far as I can tell, this code path was added by @vtjnash in 082d7d2

I have no particular beef with this code other than "it complicates the thread / task logic, so dealing with this requires understanding one more moving part, removing it makes it simpler". If it is decided that this code should rather be kept, "just in case" (which, though), that's equally fine by me. Thought perhaps a comment could be added explaining the motivation for keeping it even thought it is currently always disabled?

@fingolfin fingolfin requested a review from vtjnash May 24, 2024 09:59
@fingolfin
Copy link
Contributor Author

This needs a rebase and probably another one once PR #54569 is merged; I am happy to do those but I'd rather wait until I hear an "OK this is sensible" -- if this PR is rejected there is no point in updating it :-)

@fingolfin
Copy link
Contributor Author

@vtjnash any thoughts on this PR? Pinging you since it seems you added the JL_HAVE_SIGALTSTACK code in the first place, so may know reasons why it should be kept, or perhaps even situations were it is used after all?

My main motivation for removing it was to lower complexity when trying to understand how everything in the task system fits together, and what code is used when.

@vtjnash
Copy link
Member

vtjnash commented Jun 3, 2024

I don't have any insight into what options people use on esoteric configurations

@gbaraldi
Copy link
Member

gbaraldi commented Jun 3, 2024

I would be fine with removing it and if someone complains we can revert?

@fingolfin
Copy link
Contributor Author

@gbaraldi that's just my thought!

Disclaimer: I actually plan a follow-up PR which would slightly complicate the revert (by modifying some of the same lines as this PR), but not in a fundamental way. And I'd be happy to either assist with or even perform the revert if needed.

Nothing is ever activating this, and it probably hasn't been
tested in a while.
@fingolfin fingolfin force-pushed the mh/remove-JL_HAVE_SIGALTSTACK branch from 49ae52a to f660c95 Compare June 9, 2024 22:54
@fingolfin fingolfin merged commit ee542b6 into JuliaLang:master Jun 12, 2024
7 checks passed
@fingolfin fingolfin deleted the mh/remove-JL_HAVE_SIGALTSTACK branch June 12, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants