Append project compiler arguments after arguments from build system anddisable warning as error

Authored by gracicot on Sep 19 2017, 2:56 AM.



Arguments from project option were not applied or applied
incorrectly since D7752. This patch fixes sime problem when setting
custom arguments to the parser.

Also, since we take arguments from the build system, warning as error
became effective on the parser. I added Wno-error at the end of the
default parser arguments.

Diff Detail

R32 KDevelop
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
gracicot created this revision.Sep 19 2017, 2:56 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptSep 19 2017, 2:56 AM
kfunk added a comment.Sep 19 2017, 7:29 AM

I've almost had the same patch locally. So +1 in general.

I'm just wondering if we should rather append the "extraArguments" *after* the project-specific ones. My idea: If e.g. the CMake manager dictates -std=c++14, with my patch this would be part of extraArguments and then the standard version in use by libclang.

With your patch, we would use the -std=... argument used from the parser settings, which might not match what the user wants. IMO we should try to get everything working 'out of the box' as much as possible.

For the record, my patch:

diff --git a/plugins/custom-definesandincludes/definesandincludesmanager.cpp b/plugins/custom-definesandincludes/definesandincludesmanager.cpp
index de317ae977..02b5f48f12 100644
--- a/plugins/custom-definesandincludes/definesandincludesmanager.cpp
+++ b/plugins/custom-definesandincludes/definesandincludesmanager.cpp
@@ -356,16 +356,18 @@ QString DefinesAndIncludesManager::parserArguments(KDevelop::ProjectBaseItem* it
     Q_ASSERT(QThread::currentThread() == qApp->thread());
+    auto cfg = item->project()->projectConfiguration().data();
+    const auto parserArguments = findConfigForItem(m_settings->readPaths(cfg), item).parserArguments;
+    auto arguments = argumentsForPath(item->path(), parserArguments);
     auto buildManager = item->project()->buildSystemManager();
-    if ( buildManager ) {
-        const auto args = buildManager->extraArguments(item);
-        if (!args.isEmpty())
-            return args;
+    if (buildManager) {
+        const auto extraArguments = buildManager->extraArguments(item);
+        if (!extraArguments.isEmpty())
+            arguments += ' ' + extraArguments;
-    auto cfg = item->project()->projectConfiguration().data();
-    const auto arguments = findConfigForItem(m_settings->readPaths(cfg), item).parserArguments;
-    return argumentsForPath(item->path(), arguments);
+    return arguments;
 QString DefinesAndIncludesManager::parserArguments(const QString& path) const
kfunk requested changes to this revision.Sep 19 2017, 7:29 AM
This revision now requires changes to proceed.Sep 19 2017, 7:29 AM
kfunk retitled this revision from Append project compiler arguments after arguments from build system and disable warning as error to Append project compiler arguments after arguments from build system anddisable warning as error.Sep 19 2017, 7:29 AM
kfunk added a reviewer: apol.
apol added inline comments.Sep 19 2017, 9:36 AM

Shouldn't it get a ' ' in case it starts and ends with options?

Seems reasonable @kfunk . I'll update this patch accordingly. Although, I think there should be an option to "override" buildsystem arguments for the parser, to enable more fine grained option about what warnings the parse emit or not.

Since the intent of adding -Wno-error was to override the -Werror from the build system, it no longer make sense to add it. I'll remove it from this patch.

Also, it raises the question whether extra arguments for the build system is doing well on windows. Compiler arguments are quite different there.

gracicot updated this revision to Diff 19677.Sep 19 2017, 11:13 PM
  • Reverted Wno-error, and appending extraArguments at the end of options
mwolff added a subscriber: mwolff.Sep 20 2017, 7:47 AM

+1 if this improves the situation, but in general I was suspecting such issues while reviewing the initial changeset. I suspect we'll see more issues in the future, and I have no clear idea on how to handle that :(

kfunk accepted this revision.Sep 20 2017, 8:05 AM
This revision is now accepted and ready to land.Sep 20 2017, 8:05 AM
This revision was automatically updated to reflect the committed changes.