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.
Details
- Reviewers
kfunk mwolff - Group Reviewers
KDevelop - Commits
- R32:0c05bd55965f: Make it possible for IBuildSystem to provide extra arguments to the parser
Diff Detail
- Repository
- R32 KDevelop
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
projectmanagers/cmake/cmakemanager.cpp | ||
---|---|---|
289 | move * next to item, like above(?) | |
projectmanagers/cmake/cmakemanager.h | ||
87 | remove space after * | |
projectmanagers/custom-buildsystem/custombuildsystemplugin.h | ||
79 | dito space | |
projectmanagers/custommake/custommakemanager.h | ||
59 | dito | |
projectmanagers/qmake/qmakemanager.h | ||
63 | dito | |
projectmanagers/qmake/qmakeprojectfile.cpp | ||
292 | 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 | why LFLAGS, that sounds unnecessary for our purposes | |
295 | -isystem too? And is it really "-F" or did you mean "-D"? | |
300 | 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? |
projectmanagers/qmake/qmakeprojectfile.cpp | ||
---|---|---|
292 | 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 | Yeah, actually what I meant to do here was to filter these out. |
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. |
could we get a unit test? but otherwise lgtm, minus the one issue I found (sorry :P)
languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp | ||
---|---|---|
361 | 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? |
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.
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!
projectmanagers/qmake/qmakeprojectfile.cpp | ||
---|---|---|
300 | It should be, but this doesn't negate it :D |
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.