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

Add at-sigatomic macro (closes #12309) #12333

Closed
wants to merge 1 commit into from
Closed

Conversation

kmsquire
Copy link
Member

@@ -91,6 +91,10 @@ sigatomic_end() = ccall(:jl_sigatomic_end, Void, ())
disable_sigint(f::Function) = try sigatomic_begin(); f(); finally sigatomic_end(); end
reenable_sigint(f::Function) = try sigatomic_end(); f(); finally sigatomic_begin(); end

macro sigatomic(code)
:(try sigatomic_begin(); $code; finally sigatomic_end(); end)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be $(esc(code)).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

@vtjnash
Copy link
Member

vtjnash commented Jul 28, 2015

i think this should encourage use of the lambda form, rather than adding additional syntax:

help?> disable_sigint
search: disable_sigint

Base.disable_sigint(f::Function)

   Disable Ctrl-C handler during execution of a function, for calling
   external code that is not interrupt safe. Intended to be called
   using "do" block syntax as follows:

      disable_sigint() do
          # interrupt-unsafe code
          ...
      end

@kmsquire
Copy link
Member Author

i think this should encourage use of the lambda form, rather than adding additional syntax

Any suggestions on how it should do that?

I find this slightly cleaner for one line ccalls, and I'm generally not a fan of the lambda forms because of an preconceived notion that they might be inefficient (because, well, they use lambdas).

Any reason why the lambda forms aren't used in GTK.jl?

(See also the discussion in #12309)

@vtjnash
Copy link
Member

vtjnash commented Jul 28, 2015

Gtk.jl has complicated needs due to COPY_STACKS interfering with event loop integration (prior to that it was using the lambda form of disable_sigint).

the syntax does look pretty sensible for ccall, although there's another experiment in making it zero-cost that I want to try first before adding this

inlining can (in the future) handle the lambda transformation to make it equivalent to this, so i'm not too convinced by that argument

@kmsquire
Copy link
Member Author

inlining can (in the future) handle the lambda transformation to make it equivalent to this, so i'm not too convinced by that argument

Okay. There's been talk about various kinds of function inlining for a few years now. I'm sure we're a lot closer than we were in 2012, but you should be able to understand why I would make that statement. ;-)

Looking forward to your zero-cost solution.

@yuyichao
Copy link
Contributor

yuyichao commented May 3, 2016

Now that we have jb/functions, is this still needed?

@kmsquire
Copy link
Member Author

kmsquire commented May 3, 2016

I argued above that this is cleaner for ccalls. @vtjnash said he had a zero-cost experiment he wanted to try first, and neither of us followed up.

@vtjnash
Copy link
Member

vtjnash commented May 3, 2016

no, you can close this with your branch yyc/threads/safepoint-signal

@yuyichao
Copy link
Contributor

yuyichao commented May 3, 2016

Yep, the zero-cost approach is #16174 and I was doing my pre-PR opening search for related issues/prs.

FWIW, if there's still good reason to prefer a macro with #16174 and jb/functions, the try-catch will not be necessary anymore.

@kmsquire
Copy link
Member Author

kmsquire commented May 3, 2016

#16174 looks nice! I look forward to trying that with VideoIO sometime (although that package isn't in a very good state right now).

You can definitely close this and #12309 with the merge of #16174.

@tkelman tkelman deleted the kms/sigatomic branch May 6, 2016 16:05
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.

4 participants