D8634: Custom defines/includes: Improve handling of parser arguments

[Milian Wolff]

mwolff added a comment.


  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.

INLINE COMMENTS

> settingsmanager.cpp:42
>  namespace {
> +Utils::LanguageType languageTypes[] = {Utils::C, Utils::Cpp, Utils::OpenCl, Utils::Cuda};
> +

make this const

> settingsmanager.cpp:59
>  
> -QString parserArgumentsCPP()
> -{
> -    return QStringLiteral("parserArguments");
> -}
> -
> -QString parserArgumentsC()
> +QString parserArguments(Utils::LanguageType languageType)
>  {

rename to `configKey` or such?

> settingsmanager.cpp:79
>  }
>  }
>  

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

> settingsmanager.cpp:140
>          pathgrp.writeEntry(ConfigConstants::projectPathKey, path.path);
> -        pathgrp.writeEntry(ConfigConstants::parserArgumentsCPP(), path.parserArguments.cppArguments);
> -        pathgrp.writeEntry(ConfigConstants::parserArgumentsC(), path.parserArguments.cArguments);
> +        for (Utils::LanguageType type : languageTypes)
> +            pathgrp.writeEntry(ConfigConstants::parserArguments(type), path.parserArguments.get(type));

add braces please

> settingsmanager.cpp:174
>          path.path = pathgrp.readEntry( ConfigConstants::projectPathKey, "" );
> -        path.parserArguments.cppArguments = pathgrp.readEntry(ConfigConstants::parserArgumentsCPP(), defaultArguments().cppArguments);
> -        path.parserArguments.cArguments = pathgrp.readEntry(ConfigConstants::parserArgumentsC(), defaultArguments().cArguments);
> +        for (Utils::LanguageType type : languageTypes)
> +            path.parserArguments.set(type, pathgrp.readEntry(ConfigConstants::parserArguments(type), defaultArguments().get(type)));

dito

> settingsmanager.cpp:435
>  
> +    if (mimeType == QStringLiteral("text/x-opencl-src")) {
> +        return OpenCl;

this should probably be a separate patch

> settingsmanager.h:42
> +public:
> +    const QString& get(Utils::LanguageType languageType) const;
> +    void set(Utils::LanguageType languageType, const QString& arg);

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

> settingsmanager.h:43
> +    const QString& get(Utils::LanguageType languageType) const;
> +    void set(Utils::LanguageType languageType, const QString& arg);
> +

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

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D8634

To: aaronpuchert, #kdevelop
Cc: mwolff, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20171103/a98c6e2d/attachment-0001.html>