Make it possible for IBuildSystem to provide extra arguments to the parser
ClosedPublic

Authored by apol on Sep 9 2017, 11:07 PM.

Details

Summary

So far it was limited to defines and includes, but there's more to life
than that.
This implements it for cmake where I tested it most, also for qmake where
I'm not sure how to yet it won't be worse than ignoring the variables.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Sep 9 2017, 11:07 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptSep 9 2017, 11:07 PM
mwolff requested changes to this revision.Sep 12 2017, 3:36 PM
mwolff added a subscriber: mwolff.
mwolff added inline comments.
projectmanagers/cmake/cmakemanager.cpp
289 ↗(On Diff #19346)

move * next to item, like above(?)

projectmanagers/cmake/cmakemanager.h
87 ↗(On Diff #19346)

remove space after *

projectmanagers/custom-buildsystem/custombuildsystemplugin.h
79 ↗(On Diff #19346)

dito space

projectmanagers/custommake/custommakemanager.h
59 ↗(On Diff #19346)

dito

projectmanagers/qmake/qmakemanager.h
63 ↗(On Diff #19346)

dito

projectmanagers/qmake/qmakeprojectfile.cpp
292 ↗(On Diff #19346)

imo it should be either CFLAGS or CXXFLAGS, not a combination of the two which could lead really funky combinations that would break in magic ways... Could you maybe check at the callee site whether we are interested in C or C++ flags?

294 ↗(On Diff #19346)

why LFLAGS, that sounds unnecessary for our purposes

295 ↗(On Diff #19346)

-isystem too? And is it really "-F" or did you mean "-D"?

300 ↗(On Diff #19346)

this should be negated, no? You want to find everything that is _not_ an include path, no? Also, defines should be skipped too, thus -D above, not -F?

This revision now requires changes to proceed.Sep 12 2017, 3:36 PM
apol marked 8 inline comments as done.Sep 12 2017, 9:58 PM
apol added inline comments.
projectmanagers/qmake/qmakeprojectfile.cpp
292 ↗(On Diff #19346)

Meh, we're passing it to clang, in fact it's what clang will use to figure out if to use c++ or c parsing. I'll change to just use CXXFLAGS and it will already be better than the status quo.

295 ↗(On Diff #19346)

Yeah, actually what I meant to do here was to filter these out.

apol updated this revision to Diff 19464.Sep 12 2017, 10:00 PM
apol marked 2 inline comments as done.

Addressed Milian's comments

mwolff requested changes to this revision.Sep 13 2017, 8:15 AM
mwolff added inline comments.
projectmanagers/qmake/qmakeprojectfile.cpp
292 ↗(On Diff #19464)

You could use initializer_list here to obsolete the memory allocation for the vector:

const auto variablesToCheck = {...};
const auto prefixes = {...};
293 ↗(On Diff #19464)

isn't -isystem still missing?

This revision now requires changes to proceed.Sep 13 2017, 8:15 AM
apol updated this revision to Diff 19483.Sep 13 2017, 1:13 PM

few changes

apol marked an inline comment as done.Sep 13 2017, 1:14 PM
apol added inline comments.
projectmanagers/qmake/qmakeprojectfile.cpp
293 ↗(On Diff #19464)

I'm not sure it's a problem. Furthermore qmake isn't passing -isystem anywhere. I'd say we can try it like this and if it's a problem we include it.

mwolff requested changes to this revision.Sep 13 2017, 1:30 PM

could we get a unit test? but otherwise lgtm, minus the one issue I found (sorry :P)

languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp
361 ↗(On Diff #19483)

shouldn't we merge the build system arguments with those from the user configuration below? or is this problematic since it potentially leads to incompatible stuff?

what if the build manager returns an empty argument string? shoulnd't this then fall into the path below?

This revision now requires changes to proceed.Sep 13 2017, 1:30 PM
apol marked an inline comment as done.Sep 13 2017, 3:06 PM

Good point. I wouldn't go into merging, the whole point of this is to have the file parser use the same settings that the build system will use.

apol updated this revision to Diff 19488.Sep 13 2017, 3:06 PM

Use the configured settings if nothing was provided

mwolff accepted this revision.Sep 13 2017, 7:30 PM

ok, let's try this out :) What Could Possibly Go Wrong?

This revision is now accepted and ready to land.Sep 13 2017, 7:30 PM
brauch added a subscriber: brauch.Sep 14 2017, 9:38 AM

Re. IRC: Yes, to 5.2 I would say. We didn't release our beta yet, and I even think this counts as a bug fix ...
Thanks a lot!

This revision was automatically updated to reflect the committed changes.
brauch added inline comments.Sep 15 2017, 10:36 AM
projectmanagers/qmake/qmakeprojectfile.cpp
300 ↗(On Diff #19346)

It should be, but this doesn't negate it :D

brauch reopened this revision.Sep 15 2017, 10:57 AM

There are some arguments which we shouldn't pass on, for example I have a qmake project here which does QMAKE_CXXFLAGS += -Werror=return-type and that completely breaks everything (no highlighting any more whatsoever, parsing of all files fails, ...), Not even sure why, but I guess -Werror arguments should never be passed on in our case.

Should I make extraArguments() return a string list and add a blacklist for those? Better ideas?

This revision is now accepted and ready to land.Sep 15 2017, 10:57 AM
apol added a comment.Sep 15 2017, 12:22 PM

There are some arguments which we shouldn't pass on, for example I have a qmake project here which does QMAKE_CXXFLAGS += -Werror=return-type and that completely breaks everything (no highlighting any more whatsoever, parsing of all files fails, ...), Not even sure why, but I guess -Werror arguments should never be passed on in our case.

Should I make extraArguments() return a string list and add a blacklist for those? Better ideas?

I just tested this and it works for me, you must be seeing a different issue.

diff --git a/plugins/clang/clangsettings/clangsettingsmanager.cpp b/plugins/clang/clangsettings/clangsettingsmanager.cpp
index 517a341982..86de70a778 100644
--- a/plugins/clang/clangsettings/clangsettingsmanager.cpp
+++ b/plugins/clang/clangsettings/clangsettingsmanager.cpp
@@ -121,6 +121,7 @@ QVector<QByteArray> ParserSettings::toClangAPI() const
                    std::back_inserter(result),
                    [] (const QString &argument) { return argument.toUtf8(); });
 
+    result << "-Werror=return-type";
     return result;
 }

Ok, you're right ... I looked some more. Try "-pipe", qmake passes that here, that seems to be what breaks things if I'm not mistaken.

apol added a comment.Sep 15 2017, 4:00 PM

Ok, you're right ... I looked some more. Try "-pipe", qmake passes that here, that seems to be what breaks things if I'm not mistaken.

Nope... :/ and I tried -pipe and --pipe

apol closed this revision.Sep 21 2017, 4:49 PM