jpalecek added a comment.
In D29814#674013 <https://phabricator.kde.org/D29814#674013>, @dfaure wrote: > I'm a bit confused by the bug this is fixing. Surely this doesn't happen to all cases of crashes without autorestart enabled?? > > Also, it sounds like a null check might be enough. No. It is a crash where `s_autoRestartCommandLine` was null. The actual crash happened in the logging code, but it is also used in `startProcess` and I think going for non-null `s_autoRestartCommandLine` is actually easier than faffing with null pointer checks and special casing in the signal handler code. Even with no restart arguments, you still need `argv` to hold the executable name to restart, so why not maintain it uniformly? It only costs a few bytes of heap. INLINE COMMENTS > dfaure wrote in kcrash.cpp:107 > Should this become 1 then? Otherwise the `for` loop won't delete that new > `new char*[1]` No. The invariant is that `s_autoRestartArgc` is the number of arguments, not counting the last `null`. The `new char*[1]` is deleted after the loop. > dfaure wrote in kcrash.cpp:272 > insert(0) reads like prepend, but args is empty anyway, why not just use > append? Well I was thinking about `resize()`, then found there wasn't any. However, if we want to be clear, maybe `args = { QString() }` would be the best? (In reality, the effect should be `args = { filePath }`, but that comes with the next line.) REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D29814 To: jpalecek, #frameworks, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
