Changeset View
Standalone View
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) | ||||
mwolff: move { to its own line | |||||
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… | |||||
mwolff: please add a comment here as I requested the last time | |||||
69 | { | ||||
70 | arguments.erase(std::remove_if( | ||||
71 | arguments.begin(), | ||||
mwolff: this should use QVector::erase + std::remove_if on a mutable container | |||||
mwolff: join next line | |||||
72 | arguments.end(), | ||||
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… | |||||
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 | [](QByteArray& argument) { | ||||
mwolff: const& | |||||
brauch: this can't be const, it's modified | |||||
74 | if (argument == "-Werror") { | ||||
75 | return true; | ||||
76 | } | ||||
mwolff: move out of the loop | |||||
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? | |||||
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 | | ||||
mwolff: use QByteArrayLiteral:
const auto asError = QByteArrayLiteral("-Werror="); | |||||
mwolff: `startsWith` | |||||
78 | argument.replace("-Werror=", "-W"); | ||||
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 | | ||||
mwolff: remove empty line | |||||
mwolff: add a comment:
```
// replace -Werror=foo with -Wfoo
``` | |||||
80 | return false; | ||||
81 | } | ||||
82 | ), arguments.end()); | ||||
83 | } | ||||
84 | | ||||
68 | QVector<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 | } | ||
brauch: not sure what std::move accomplishes here? | |||||
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… | |||||
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 | | |||
111 | void 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 |
move { to its own line