D16218: [KDevelop/Core]: safe signal-handler implementation (WIP)

[Kevin Funk]

kfunk added a comment.


  In D16218#343380 <https://phabricator.kde.org/D16218#343380>, @rjvbb wrote:
  
  > > - Why does this patch look so much more verbose than the solution proposed on http://doc.qt.io/qt-5/unix-signals.html?
  >
  > Probably because it's a patch against existing code, which needs changes due to the fact that I thought I'd had to avoid static globals.
  
  
  Not sure what you mean by that, but see my last remark in this reply.
  
  >> - Why the alternative implementations? Why do we need to the non-QSocketNotifier variant? You realize that every "alternative solution" or `#ifdef` puts more maintenance burden on us?
  > 
  > Who said I wanted to have both committed - the title has `WIP` in it for a reason?! I've explained why there are 2 alternatives (re-read goal 2). There's another reason why avoiding QSocketNotifier could be useful: using a semaphore doesn't require spending 2 file descriptors on something you might never need (it isn't hard to run into the open fd limit with KDevelop, esp. when not running Linux).
  
  This KDevelop problem is not solved by saving those 2 of those file descriptors.
  
  >> Should be a separate patch, I suppose that's close to a one-line patch against the current code base.
  > 
  > Which is also why it could be just as well part of a complete overhaul of the whole signal handling stuff, IMHO.
  
  No, because we now have to discuss several different things as part of one review again, a repeating pattern which makes these discussion so dreadful and exhausting.
  
  1. There's this one behavioral change you wanted, which would fix the bug, a one- or two-line patch, which could be possibly even added to 5.3.
  2. Then there's a refactoring patch which should go to master branch, way more invasive and bound to fail for some users on the first iteration.
  
  >> Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).
  > 
  > Which goes against the principle used otherwise to avoid global symbols and stuff everything of that sort into classes...
  
  There's no single "global symbol" (and I guess by that you mean "global static"), only class statics. And this implementation is undeniably more easy to read than a scattered dump of blocks of signal-handling related code into an existing class. Not doing so again makes it harder to reason about the code and increases complexity.

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20181015/285586d7/attachment.html>