<table><tr><td style="">rjvbb marked an inline comment as done.<br />rjvbb added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D16218">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16218#inline-88197">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aaronpuchert</span> wrote in <span style="color: #4b4d51; font-weight: bold;">core.cpp:58</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I think it's better to use <tt style="background: #ebebeb; font-size: 13px;">std::atomic</tt> here, <tt style="background: #ebebeb; font-size: 13px;">volatile</tt> should be a thing of the past.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">From what I've read std::atomic is safe only if its <tt style="background: #ebebeb; font-size: 13px;">is_lock_free</tt> property is true. If not, sig_atomic_t is the type of choice. (You can't lock a mutex in a signal handler.)</p>

<p style="padding: 0; margin: 8px;">As to the volatile property, I just copied that from <tt style="background: #ebebeb; font-size: 13px;">handlingSignal</tt>.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16218#inline-88205">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aaronpuchert</span> wrote in <span style="color: #4b4d51; font-weight: bold;">core.cpp:68-69</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Is this supposed to be thread-safe? Because it isn't. If it should be thread-safe, make <tt style="background: #ebebeb; font-size: 13px;">handlingSignal</tt> a <tt style="background: #ebebeb; font-size: 13px;">std::atomic</tt> (non-volatile) and use <tt style="background: #ebebeb; font-size: 13px;">compare_exchange_strong</tt>.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I don't think there's a need for that.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16218#inline-88207">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aaronpuchert</span> wrote in <span style="color: #4b4d51; font-weight: bold;">core.cpp:79-83</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Shouldn't this be in the <tt style="background: #ebebeb; font-size: 13px;">if</tt> as well or what am I missing?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">No, the shutdown procedure seems to be designed to be a 2-stage process. The first interrupt will trigger a regular shutdown procedure (inside the if) and lead to exit if all goes well. If that process blocks or if another signal comes in before it can terminate, the part outside the if is executed.</p>

<p style="padding: 0; margin: 8px;">In its current form you could indeed put the SIG_DFL statement inside or before the if. In my original proposal there was a small addition here: shutdown of just the core plugin, evidently before restoring the default signal handler. That change will be resubmitted so I'd rather not move statements now that I'd have to move back again.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16218#inline-88201">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aaronpuchert</span> wrote in <span style="color: #4b4d51; font-weight: bold;">core.cpp:86</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">If I read the standard correctly, this needs C linkage, so wrap it in <tt style="background: #ebebeb; font-size: 13px;">extern "C" {...}</tt>.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Does that do anything other than not mangling function names?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16218#inline-88206">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aaronpuchert</span> wrote in <span style="color: #4b4d51; font-weight: bold;">core.cpp:90</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Can you use this in a signal handler?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">there's a good chance it will allocate memory, indeed.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D16218">https://phabricator.kde.org/D16218</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, kfunk<br /><strong>Cc: </strong>aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight<br /></div>