Differential D14710 Diff 39352 debugger/pdblauncher.cpp

Changeset View

Standalone View

View Options

debugger/pdblauncher.cpp

Show All 32 Lines
33#include <KLocalizedString> 33​#include <KLocalizedString>
34#include <KMessageBox> 34​#include <KMessageBox>
35#include <KParts/MainWindow> 35​#include <KParts/MainWindow>
36#include <KConfigGroup> 36​#include <KConfigGroup>
37#include <QFileInfo> 37​#include <QFileInfo>
38 38​
39#include <QDebug> 39​#include <QDebug>
40#include "debuggerdebug.h" 40​#include "debuggerdebug.h"
41​#include <util/environmentprofilelist.h>
41 42​
42 43​
43namespace Python { 44​namespace Python {
44 45​
45PdbLauncher::PdbLauncher() 46​PdbLauncher::PdbLauncher()
46{ 47​{
47 48​
48} 49​}
▲ Show 20 Lines • Show All 61 Lines • ▼ Show 20 Line(s)110 {
110 wd = QUrl::fromLocalFile( QFileInfo( scriptUrl.toLocalFile() ).absolutePath() ); 111​ wd = QUrl::fromLocalFile( QFileInfo( scriptUrl.toLocalFile() ).absolutePath() );
111 } 112​ }
112 113​
113 DebugJob* job = new DebugJob(); 114​ DebugJob* job = new DebugJob();
114 job->m_scriptUrl = scriptUrl; 115​ job->m_scriptUrl = scriptUrl;
115 job->m_interpreter = interpreter; 116​ job->m_interpreter = interpreter;
116 job->m_args = iface->arguments(cfg, err); 117​ job->m_args = iface->arguments(cfg, err);
117 job->m_workingDirectory = wd; 118​ job->m_workingDirectory = wd;
119​
120​ const KDevelop::EnvironmentProfileList environmentProfiles(KSharedConfig::openConfig());
121​ QString envProfileName = iface->environmentProfileName(cfg);
122​
123​ if (envProfileName.isEmpty()) {
124​ qCWarning(KDEV_PYTHON_DEBUGGER) << i18n("No environment profile specified, looks like a broken "
apolUnsubmitted
Not Done

We usually don't translate warnings that won't show on the UI.

apol: We usually don't translate warnings that won't show on the UI.
KebianizaoUnsubmitted
Not Done

ACK.

Kebianizao: ACK.
125​ "configuration, please check run configuration '%1'. "
126​ "Using default environment profile.", cfg->name());
127​ envProfileName = environmentProfiles.defaultProfileName();
128​ }
129​
130​ auto environment = environmentProfiles.variables(envProfileName);
apolUnsubmitted
Not Done

const

apol: const
KebianizaoUnsubmitted
Not Done

ACK.

Kebianizao: ACK.
131​
132​ QMap<QString, QString>::const_iterator i;
133​
134​ for(i = environment.begin(); i != environment.end(); ++i)
vkorneevUnsubmitted
Not Done

Sorry, I'm not actually the reviewer, but may I ask you, why don't use

for(const auto &i: environment) {

here? Just interested if I miss something.

vkorneev: Sorry, I'm not actually the reviewer, but may I ask you, why don't use ``` for(const auto &i…
KebianizaoUnsubmitted
Not Done

I well may wrong, but what I can tell is that:
environment is a QMap<QString,QString>
when I do:
for(const auto &i: environment), i type is QString so I should have to write this chunk as:

for(const auto &i : environment)
{
    job->m_environment << environment[i];
}

I don't know if this is what you were thinking.
Now I got to this, maybe this is a good idea. I can submit the patch with this change if you like.
Reference: https://stackoverflow.com/questions/8517853/iterating-over-a-qmap-with-for#8529237

Kebianizao: I well may wrong, but what I can tell is that: environment is a QMap<QString,QString> when I do…
brauchUnsubmitted
Not Done

Either way is fine imo, I would maybe just initialize the iterator inline with auto:

for (auto i = environment.begin() ...)

It really doesn't matter much though.

brauch: Either way is fine imo, I would maybe just initialize the iterator inline with auto: ``` for…
KebianizaoUnsubmitted
Not Done

Sorry I meant:

for(const auto &i : environment)
{
         job->m_environment << i + "=" + environment[i];
}

But anyway, I'll stick with what @brauch suggests.

Kebianizao: Sorry I meant: for(const auto &i : environment) { job->m_environment << i + "="…
vkorneevUnsubmitted
Not Done

But anyway, I'll stick with what @brauch suggests.

Sure. The version suggested by @brauch is the best one here. The one I was thinking about is wrong, because dereferencing QMap`s const_iterator returns value, not the pair or key (I didn't notice that).

@brauch's version is right and the scope of the iterator is more limited than in your first version, so it's better. Although, if you're not modifying anything, maybe you can use cbegin() and cend() instead of begin() and end(), but as @brauch said it really doesn't matter much.

vkorneev: > But anyway, I'll stick with what @brauch suggests. Sure. The version suggested by @brauch is…
135​ {
136​ QString oneEnvVariable = i.key() + "=" + i.value();
137​ job->m_environment << oneEnvVariable;
138​ }
139​
118 QList<KJob*> l; 140​ QList<KJob*> l;
119 l << job; 141​ l << job;
120 return new KDevelop::ExecuteCompositeJob( KDevelop::ICore::self()->runController(), l ); 142​ return new KDevelop::ExecuteCompositeJob( KDevelop::ICore::self()->runController(), l );
121 } 143​ }
122 qCDebug(KDEV_PYTHON_DEBUGGER) << "unknown launch mode"; 144​ qCDebug(KDEV_PYTHON_DEBUGGER) << "unknown launch mode";
123 return nullptr; 145​ return nullptr;
124} 146​}
125 147​
126QStringList PdbLauncher::supportedModes() const 148​QStringList PdbLauncher::supportedModes() const
127{ 149​{
128 return QStringList() << "debug"; 150​ return QStringList() << "debug";
129} 151​}
130 152​
131} 153​}