| kfunk | |
| dfaure |
| Frameworks |
This patch makes kfiledialog functions that did not already do so test the value of d->native before referencing d->w. This is important because if d->native is non-null then d->w will be null and referencing it cause an immediate crash. This was the case on Windows builds of okular when the file chooser was opened.
The bug and patch were previously described here: https://bugs.kde.org/show_bug.cgi?id=364086
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Again we have a patch against no repo, so i don't even know how to read this patch because i can't read the rest of the file.
| src/kio/kfiledialog.cpp | ||
|---|---|---|
| 879 | nullptr here and in other places | |
| src/kio/kfiledialog.cpp | ||
|---|---|---|
| 607 | Should we rather check for !d->w here and below? Would make more sense IMO. | |
Agreed that would be more robust. In writing the patch I was seeking consistency with those functions that already did the test, so those would also need to be updated. Are there any situations where the two tests would yield a different result, ie d->native is true and d->w is non-null?
Probably not. But I think it makes more sense to check the pointer you're actually going to dereference in the next statement.
Could you update the patch? And also fix the nullptr issues?
Following @kfunk comments, I've changed the patch to test d->w rather than d->native, and used nullptr instead of 0 where appropriate.
Apparently updating the diff has had the side-effect of:
removed R239 KDELibs4Support as the repository for this revision.
Please excuse my bumbling - I don't know why that happened or how to repair it. :(
Found a couple of places in old code where d->w made more sense than d->native to test. Also figured out why the repository got lost last time...
Why did you remove all the early-returns? Was that the case before in one of your earlier patches?
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.
Interesting claim that "not using early returns" makes the code more readable -- please have a look at the discussion on http://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement -- I agree that you could consider KFileDialog having modes of operations though -- there having early returns could limit the extensibility in case you ever wanted to introduce another mode.
Anyhow, I don't care too much for this deprecated code base. This patch looks fine semantically, and fixes a bug => so let's go for it.
I'd wait for another +1 from someone else.