D7752: Make it possible for IBuildSystem to provide extra arguments to the parser

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> cmakemanager.cpp:289
>  
> +QString CMakeManager::extraArguments(KDevelop::ProjectBaseItem* item) const
> +{

move `*` next to item, like above(?)

> cmakemanager.h:87
>      QHash<QString, QString> defines(KDevelop::ProjectBaseItem *) const override;
> +    QString extraArguments(KDevelop::ProjectBaseItem * item) const override;
>  

remove space after `*`

> custombuildsystemplugin.h:79
>      KDevelop::Path::List frameworkDirectories( KDevelop::ProjectBaseItem* ) const override;
> +    QString extraArguments(KDevelop::ProjectBaseItem * item) const override;
>      bool removeFilesFromTargets( const QList<KDevelop::ProjectFileItem*>& ) override;

dito space

> custommakemanager.h:59
>  
> +    QString extraArguments(KDevelop::ProjectBaseItem * item) const override;
> +

dito

> qmakemanager.h:63
>      QHash<QString,QString> defines(KDevelop::ProjectBaseItem*) const override;
> +    QString extraArguments(KDevelop::ProjectBaseItem * item) const override;
>      bool hasBuildInfo(KDevelop::ProjectBaseItem*) const override;

dito

> qmakeprojectfile.cpp:292
> +{
> +    const auto variablesToCheck = {QStringLiteral("QMAKE_CFLAGS"),
> +                                QStringLiteral("QMAKE_CXXFLAGS"),

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?

> qmakeprojectfile.cpp:294
> +                                QStringLiteral("QMAKE_CXXFLAGS"),
> +                                QStringLiteral("QMAKE_LFLAGS")};
> +    const QVector<QLatin1String> prefixes = { QLatin1String("-F"), QLatin1String("-iframework"), QLatin1String("-I") };

why LFLAGS, that sounds unnecessary for our purposes

> qmakeprojectfile.cpp:295
> +                                QStringLiteral("QMAKE_LFLAGS")};
> +    const QVector<QLatin1String> prefixes = { QLatin1String("-F"), QLatin1String("-iframework"), QLatin1String("-I") };
> +    QStringList args;

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

> qmakeprojectfile.cpp:300
> +            for (const auto& prefix: prefixes) {
> +                if (arg.startsWith(prefix)) {
> +                    args << arg;

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`?

REPOSITORY
  R32 KDevelop

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

To: apol, #kdevelop, kfunk, mwolff
Cc: mwolff, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170912/1756314c/attachment.html>