jonathans added a comment.
I wrote the first patch to be as minimal as possible and to be consistent with the previous coding style. I therefore left the early returns in place. I wrote the latest patch based on my interpretation of your (@kfunk) feedback that it makes more sense to test d->w than d->native. Because implementing this involved a change to the existing coding style I took the liberty of writing it according to what I believe to be good coding practice. Early returns are suitable for dealing with erroneous or trivial cases, but less so when dealing with modes of operation, as in this code. They interrupt the logical flow of the code, and because the conditionally executed code no longer sits inside an indented block, make it less evident to the reader that it is conditionally executed. Actually thinking about this code a bit more closely, I would now take the position that the earlier practice of only testing d->native is more sensible. What is happening here is that kfiledialog has two modes of operation: native and non-native. This mode is reflected in the two variables d->native and d->w, of which only one should ever be non-null. Which of the two variable the code tests now (ie with the latest patch) varies from case to case, which makes it inconsistent. Moreover the meaning of d->native is self-explanatory, unlike d->w. Although testing d->w would avoid segmentation faults if both d->native or d->w were null, the fact is that if that ever happened there would have to be a serious problem anyway and the code would be obviously broken. Trapping a segfault with a debugger would actually make it easier to locate the problem than, for instance, a file dialog window simply failing to be displayed. But that's all going off on a tangent. Whatever your call is on this case I'm happy to accept. REPOSITORY R239 KDELibs4Support REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure, kfunk Cc: kfunk, aacid
