Differential D7979 Diff 20458 plugins/clang/duchain/parsesession.cpp

Changeset View

Standalone View

View Options

plugins/clang/duchain/parsesession.cpp

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Line(s)53{
60 foreach (const QString& arg, extraArgs) { 60​ foreach (const QString& arg, extraArgs) {
61 result << arg.toLatin1(); 61​ result << arg.toLatin1();
62 } 62​ }
63 clangDebug() << "Passing extra arguments to clang:" << result; 63​ clangDebug() << "Passing extra arguments to clang:" << result;
64 64​
65 return result; 65​ return result;
66} 66​}
67 67​
68​void sanitizeArguments(QVector<QByteArray>& arguments)
mwolffUnsubmitted
Not Done

move { to its own line

mwolff: move { to its own line
mwolffUnsubmitted
Not Done

please add a comment explaining why this is needed (it's clear now, but for the future it would be helpful to have a short reminder). Also note down why you handle both, -Werror and -Werror= (give an example for the latter). It took me a bit to remember what that is for

mwolff: please add a comment explaining why this is needed (it's clear now, but for the future it would…
mwolffUnsubmitted
Not Done

please add a comment here as I requested the last time

mwolff: please add a comment here as I requested the last time
69​{
70​ // We remove the -Werror flag, and replace -Werror=foo by -Wfoo.
71​ // Warning as error may cause problem to the clang parser.
mwolffUnsubmitted
Not Done

this should use QVector::erase + std::remove_if on a mutable container

mwolff: this should use QVector::erase + std::remove_if on a mutable container
mwolffUnsubmitted
Not Done

join next line

mwolff: join next line
72​ const auto asError = QByteArrayLiteral("-Werror=");
mwolffUnsubmitted
Not Done

hmmm I like the brevity of this code, I'm not too sure what others think. mutating from within a remove_if callback? Haven't seen this before, but it should work so let's keep it this way. please do add a comment here though.

mwolff: hmmm I like the brevity of this code, I'm not too sure what others think. mutating from within…
mwolffUnsubmitted
Not Done

discussing this with a colleague, he also agrees that this is quite uncommon. Could you please separate the code using two algorithms for readability? Sure, it will be marginally slower than, but easier to grasp for readers and also more failsafe. i.e. split it up like so:

// step one: remove -Werror
erase + remove_if
// step two: replace -Werror= with -W
transform or for_each

Actually, thinking out loud - could you simply always only do a transform/foreach and replace the -Werror args with empty strings?

mwolff: discussing this with a colleague, he also agrees that this is quite uncommon. Could you please…
73​ for (auto& argument : arguments) {
mwolffUnsubmitted
Not Done

const&

mwolff: const&
brauchUnsubmitted
Not Done

this can't be const, it's modified

brauch: this can't be const, it's modified
74​ if (argument == "-Werror") {
75​ argument.clear();
76​ } else {
mwolffUnsubmitted
Not Done

move out of the loop

mwolff: move out of the loop
gracicotAuthorUnsubmitted
Not Done

Well... I don't really need it outside the loop why should I put it in a wider scope?

gracicot: Well... I don't really need it outside the loop why should I put it in a wider scope?
mwolffUnsubmitted
Not Done

because it's constant and doesn't depend on the loop arguments. It's always a good idea to hoist stuff out of the loop in such situations. Maybe the compiler is smart enough, but doing it manually doesn't hurt and ensures it's always hoisted out.

mwolff: because it's constant and doesn't depend on the loop arguments. It's always a good idea to…
77​ if (argument.startsWith(asError)) {
mwolffUnsubmitted
Not Done

use QByteArrayLiteral:

const auto asError = QByteArrayLiteral("-Werror=");
mwolff: use QByteArrayLiteral: const auto asError = QByteArrayLiteral("-Werror=");
mwolffUnsubmitted
Not Done

startsWith

mwolff: `startsWith`
78​ // replace -Werror=foo by -Wfoo
mwolffUnsubmitted
Not Done

only on start, no? I.e. we don't want to replace that somewhere in the middle. So instead do a startsWith check and then replace the start range with "-W"

mwolff: only on start, no? I.e. we don't want to replace that somewhere in the middle. So instead do a…
79​ argument.remove(2, asError.length() - 2);
mwolffUnsubmitted
Not Done

remove empty line

mwolff: remove empty line
mwolffUnsubmitted
Not Done

add a comment:

// replace -Werror=foo with -Wfoo
mwolff: add a comment: ``` // replace -Werror=foo with -Wfoo ```
80​ }
81​ }
82​ }
83​}
84​
68QVector<QByteArray> argsForSession(const QString& path, ParseSessionData::Options options, const ParserSettings& parserSettings) 85​QVector<QByteArray> argsForSession(const QString& path, ParseSessionData::Options options, const ParserSettings& parserSettings)
69{ 86​{
70 QMimeDatabase db; 87​ QMimeDatabase db;
71 if(db.mimeTypeForFile(path).name() == QStringLiteral("text/x-objcsrc")) { 88​ if(db.mimeTypeForFile(path).name() == QStringLiteral("text/x-objcsrc")) {
72 return {QByteArrayLiteral("-xobjective-c++")}; 89​ return {QByteArrayLiteral("-xobjective-c++")};
73 } 90​ }
74 91​
75 // TODO: No proper mime type detection possible yet 92​ // TODO: No proper mime type detection possible yet
76 // cf. https://bugs.freedesktop.org/show_bug.cgi?id=26913 93​ // cf. https://bugs.freedesktop.org/show_bug.cgi?id=26913
77 if (path.endsWith(QLatin1String(".cl"), Qt::CaseInsensitive)) { 94​ if (path.endsWith(QLatin1String(".cl"), Qt::CaseInsensitive)) {
78 return {QByteArrayLiteral("-xcl")}; 95​ return {QByteArrayLiteral("-xcl")};
79 } 96​ }
80 97​
81 // TODO: No proper mime type detection possible yet 98​ // TODO: No proper mime type detection possible yet
82 // cf. https://bugs.freedesktop.org/show_bug.cgi?id=23700 99​ // cf. https://bugs.freedesktop.org/show_bug.cgi?id=23700
83 if (path.endsWith(QLatin1String(".cu"), Qt::CaseInsensitive)) { 100​ if (path.endsWith(QLatin1String(".cu"), Qt::CaseInsensitive)) {
84 return {QByteArrayLiteral("-xcuda")}; 101​ return {QByteArrayLiteral("-xcuda")};
85 } 102​ }
86 103​
87 if (parserSettings.parserOptions.isEmpty()) { 104​ if (parserSettings.parserOptions.isEmpty()) {
88 // The parserOptions can be empty for some unit tests that use ParseSession directly 105​ // The parserOptions can be empty for some unit tests that use ParseSession directly
89 auto defaultArguments = ClangSettingsManager::self()->parserSettings(path).toClangAPI(); 106​ auto defaultArguments = ClangSettingsManager::self()->parserSettings(path).toClangAPI();
107​
90 defaultArguments.append(QByteArrayLiteral("-nostdinc")); 108​ defaultArguments.append(QByteArrayLiteral("-nostdinc"));
91 defaultArguments.append(QByteArrayLiteral("-nostdinc++")); 109​ defaultArguments.append(QByteArrayLiteral("-nostdinc++"));
92 defaultArguments.append(QByteArrayLiteral("-xc++")); 110​ defaultArguments.append(QByteArrayLiteral("-xc++"));
111​
112​ sanitizeArguments(defaultArguments);
93 return defaultArguments; 113​ return defaultArguments;
94 } 114​ }
brauchUnsubmitted
Not Done

not sure what std::move accomplishes here?

brauch: not sure what std::move accomplishes here?
mwolffUnsubmitted
Not Done

it's just overly complicated, imo. a plain remove_if + erase would achieve the same, without moving and without constructing a second container, thus probably being faster too (and less code!)

mwolff: it's just overly complicated, imo. a plain remove_if + erase would achieve the same, without…
gracicotAuthorUnsubmitted
Not Done

I was focused on returning a new, filtered container, instead of just changing the current one. Sorry, I'll fix it soon. Also, I just realized I take the container by const ref, so this std::move is completely ineffective indeed.

gracicot: I was focused on returning a new, filtered container, instead of just changing the current one.
95 115​
96 auto result = parserSettings.toClangAPI(); 116​ auto result = parserSettings.toClangAPI();
97 result.append(QByteArrayLiteral("-nostdinc")); 117​ result.append(QByteArrayLiteral("-nostdinc"));
98 if (parserSettings.isCpp()) { 118​ if (parserSettings.isCpp()) {
99 result.append(QByteArrayLiteral("-nostdinc++")); 119​ result.append(QByteArrayLiteral("-nostdinc++"));
100 } 120​ }
101 121​
102 if (options & ParseSessionData::PrecompiledHeader) { 122​ if (options & ParseSessionData::PrecompiledHeader) {
103 result.append(parserSettings.isCpp() ? QByteArrayLiteral("-xc++-header") : QByteArrayLiteral("-xc-header")); 123​ result.append(parserSettings.isCpp() ? QByteArrayLiteral("-xc++-header") : QByteArrayLiteral("-xc-header"));
124​
125​ sanitizeArguments(result);
104 return result; 126​ return result;
105 } 127​ }
106 128​
107 result.append(parserSettings.isCpp() ? QByteArrayLiteral("-xc++") : QByteArrayLiteral("-xc")); 129​ result.append(parserSettings.isCpp() ? QByteArrayLiteral("-xc++") : QByteArrayLiteral("-xc"));
130​
131​ sanitizeArguments(result);
108 return result; 132​ return result;
109} 133​}
110 134​
111void addIncludes(QVector<const char*>* args, QVector<QByteArray>* otherArgs, 135​void addIncludes(QVector<const char*>* args, QVector<QByteArray>* otherArgs,
112 const Path::List& includes, const char* cliSwitch) 136​ const Path::List& includes, const char* cliSwitch)
113{ 137​{
114 foreach (const Path& url, includes) { 138​ foreach (const Path& url, includes) {
115 if (url.isEmpty()) { 139​ if (url.isEmpty()) {
▲ Show 20 Lines • Show All 395 Lines • Show Last 20 Lines