D4929: DrKonqi : lldb support

[Kevin Funk]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20170307/f0039059/attachment.html>