kfunk requested changes to this revision. kfunk added a comment. This revision now requires changes to proceed.
So, plasma-workspace is usually way out of my comfort zone, I'm commenting anyway since RJVB urged me to do it. See my notes. INLINE COMMENTS > backtracegenerator.cpp:62 > + // than waiting a potentially very long time for it to heed the > kill() request. > + m_proc->deleteLater(); > + } else { Please do this only if `lldb` is used then. No need to change code which apparently worked fine for years. Also, did you try `QProcess::terminate` instead? PS: `QProcess` gets unhappy when being deleted while still running (=> runtime warnings). PPS: Please read my other advice about quitting LLDB below, too > backtracegenerator.cpp:144 > emit newLine(line); > + line = line.simplified(); > + if (line.startsWith(QLatin1String("Process ")) && > line.endsWith(QLatin1String(" detached"))) { This whole logic looks really cumbersome. Is there really no way to exit LLDB cleanly after detach? http://stackoverflow.com/questions/26267289/how-can-i-exit-lldb-after-running-commands-with-o suggests there is: lldb /bin/ls -o "run" -o "script import os; os._exit(1)" I take it not everyone's got Python on they system, but this works for me as well: lldb -p $(pidof kate) -o detach -o quit Just put that into the `-o quit` in the lldbrc? > backtracegenerator.h:87 > QString m_parsedBacktrace; > + bool m_lldbDetached; > Looks pretty unclean to have a backend-specific variable around here. > AppleTerminal:1 > +#!/bin/sh > + Please explain why this file is needed(?) > lldbrc:9 > +ExecInputFile=%tempfile > +BatchCommands=set set term-width 200\nthread info\nbt all\ndetach `set set`? Really? > drkonqibackends.cpp:165 > KConfigGroup config(KSharedConfig::openConfig(), "DrKonqi"); > -#ifndef Q_OS_WIN > +#if defined(__MAC_OS_X_VERSION_MAX_ALLOWED) && > __MAC_OS_X_VERSION_MAX_ALLOWED > 1070 > + QString defaultDebuggerName = config.readEntry("Debugger", > QString("lldb")); Why these special conditions? Needs comments. Whether to use or not to use lldb on a particular OS X version should be runtime decision, too. > backtraceparserlldb.cpp:26 > +public: > + BacktraceLineLldb(const QString & line); > +}; Style: Use `const QString &line` More of these issues in other lines > backtraceparserlldb.h:28 > +public: > + explicit BacktraceParserLldb(QObject *parent = 0); > + `nullptr` > backtraceparserlldb.h:31 > +protected Q_SLOTS: > + virtual void newLine(const QString & lineStr); > + Use `override`, strip `virtual` REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D4929 To: rjvbb, #plasma_workspaces, kfunk Cc: kfunk, mart, broulik, kde-mac, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol