Custom defines/includes: Improve handling of parser arguments
ClosedPublic

Authored by aaronpuchert on Nov 3 2017, 2:05 AM.

Details

Summary

This fixes numerous bugs:

  • Parser arguments for OpenCL and Cuda were not persisted.
  • OpenCL standards are defined via -cl-std=, not -std=. That broke some assumptions.
  • Custom arguments were detected (regardless of the actual language) by replacing the standard with c++11 and comparing against the default arguments for C++. This accidentally worked, but will fail if someone changes the default arguments.

Most of the hacks were left in place to keep the changes small, though
I'd argue that a proper solution needs to be found for most if the
issues.

To make the handling of parser arguments easier, I tried to use the
existing Utils::LanguageType enumeration as often as possible. Instead
of initializing the ParserArguments with an aggregate, I assigned the
arguments for each language separately to ensure there are no mix-ups in
the order.

I'm not very fond of the ParserArguments class/struct-hybrid, but I'm
not quite sure in which direction to go there.

Although KDevelop itself runs fine with these changes, there are some
problems in the TestDefinesAndIncludes, where the parser arguments,
includes or defines on an inode/directory are queried. But what are
these supposed to be? Directories can't be compiled.

Diff Detail

Repository
R32 KDevelop
Branch
parser-arguments
Lint
No Linters Available
Unit
No Unit Test Coverage
aaronpuchert created this revision.Nov 3 2017, 2:05 AM
mwolff added a subscriber: mwolff.Nov 3 2017, 10:49 AM

Overall I'm in favor of where this is heading. I've added a bunch of comments, but then I figured - couldn't we simply replace ParserArguments with e.g. a QHash<Utils::LanguageType, QString>? Or, better yet, we make it a QString[Utils::LanguageType::Other]? That would simplify a lot of the code you are changing/adding, while still enabling your approach. The advantage is that these types are easily iteratable and thus isAnyEmpty becomes a simple std::any_of all, the .get() and .set() calls get replaced by operator[] calls, and you'd still be able to use brace-initialization, while having to write less code overall.

plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp
42 ↗(On Diff #21821)

make this const

59 ↗(On Diff #21821)

rename to configKey or such?

79 ↗(On Diff #21821)

move this down, such that the anon namespace includes all util functions, esp. the ones you are touching.

140 ↗(On Diff #21821)

add braces please

174 ↗(On Diff #21821)

dito

435 ↗(On Diff #21821)

this should probably be a separate patch

plugins/custom-definesandincludes/compilerprovider/settingsmanager.h
42 ↗(On Diff #21821)

rename to argumentsForLanguage, or make it an operator[]() const

43 ↗(On Diff #21821)

rename to setArgumentsForLanguage, and rename arg to arguments. alternatively think about adding an QString& operator[]()

I'm going to address the comments I didn't reply to.

One problem remains: in test_definesandincludes, some of the methods (.includes(), .defines(), .parserArguments()) are called on directories, which can't really work. (Directories aren't compiled, first of all, and of course they can contain files of different languages.)

Even before this patch, inode/directory is mapped to Utils::Other in Utils::languageType(), for which we return no parser arguments. When asked for includes/defines, we just fail with Q_UNREACHABLE().

I'm not sure how to proceed there. The tests were probably written at a time when only C/C++ were supported, and there was no issue. Of course we can still return the C++ flags/includes/defines, but why should we?

plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp
42 ↗(On Diff #21821)

Maybe even constexpr?

435 ↗(On Diff #21821)
plugins/custom-definesandincludes/compilerprovider/settingsmanager.h
42 ↗(On Diff #21821)

In fact I had this as operator[] originally, but the problem was that get and set have slightly different domains as of now. Meaning that there are some languages for which we don't have a setting, but instead just return an empty string. (See argumentsForPath in plugins/custom-definesandincludes/definesandincludesmanager.cpp)

I agree that this is a bit weird, but I'm not sure how to handle it. Do we want to persist options for Objective C as well? And why do we have Other?

apol requested changes to this revision.Nov 7 2017, 11:58 AM
apol added a subscriber: apol.

Waiting for Milian's issues to be addressed

This revision now requires changes to proceed.Nov 7 2017, 11:58 AM
aaronpuchert marked 9 inline comments as done.

Incorporated review by Milian:

  • Used array instead of single named members for parser arguments.
  • Replaced get() and set() by operator[].
  • Added default arguments for Objective C to make sure isAnyEmpty still works as intended.

I figured - couldn't we simply replace ParserArguments with e.g. a QHash<Utils::LanguageType, QString>? Or, better yet, we make it a QString[Utils::LanguageType::Other]?

There is still that flag parseAmbiguousAsCPP, but I like the idea of using an array. Maybe we can get rid of the flag, but I wouldn't want to do too much in one change.

you'd still be able to use brace-initialization

As stated in the commit message, I don't think a bare-bones initializer list of strings is very readable, because it's not clear which arguments belong to which language when reading it. However, I don't see any issue with an initializer list consisting of std::pair<Utils::LanguageType, QString>, if you like that.

plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp
59 ↗(On Diff #21821)

That's already used in line 45. I could name it parserArgumentsKey.

79 ↗(On Diff #21821)

This closes the ConfigConstants namespace, so I guess I'm going to leave it there.

The anonymous namespace is closed on line 266.

mwolff requested changes to this revision.Nov 28 2017, 8:08 AM

thanks for the update, some small nits then this is fine to go in.

plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp
73

make this Utils::Other, such that we get a compile warning when we extend the enum later

144

auto?

179

auto?

184

auto?

plugins/custom-definesandincludes/compilerprovider/settingsmanager.h
54

missing word: "the"

plugins/custom-definesandincludes/kcm_widget/parserwidget.cpp
60

dito, explicitly mention Other here

This revision now requires changes to proceed.Nov 28 2017, 8:08 AM
aaronpuchert marked an inline comment as done.

Made sure switches over enums cover all values, and used more auto.

aaronpuchert marked 4 inline comments as done.Nov 28 2017, 9:50 PM

Good idea, -Wswitch is a nice tool. But it doesn't seem activated in the builds, at least with Clang.

aaronpuchert marked an inline comment as done.Nov 28 2017, 11:35 PM
mwolff accepted this revision.Nov 30 2017, 9:44 AM

thanks

This revision was automatically updated to reflect the committed changes.