D11136: Make sure we use the same compiler settings as the project is by default

[Milian Wolff]

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

INLINE COMMENTS

> custombuildsystemplugin.cpp:201
> +    KConfigGroup g = configuration( item->project() );
> +    const QString subgrpname = ConfigConstants::toolGroupPrefix() + QLatin1String("Build");
> +    KConfigGroup grp = g.group( subgrpname );

this is wrong, you'll have to introduce another key for this, building could e.g. be "make" and not the compiler itself

feel free to leave this not-implemented and return nothing for now, and then get some sane default in the IADM

> compilerfactories.h:35
>      QString name() const override;
> +    bool isSupported(const KDevelop::Path & path) const override;
>  

here and below: style: space before &

> compilerprovider.cpp:155
> +
> +            auto path = p->buildSystemManager()->compiler(target);
> +            qCDebug(DEFINESANDINCLUDES) << "found compiler" << path;

I find it odd that this is the only place where this new API is being used, no? shouldn't it also be used elswhere? like, when we don't really use runtimes at all?

> compilerprovider.cpp:157
> +            qCDebug(DEFINESANDINCLUDES) << "found compiler" << path;
> +            if (!path.isEmpty())
> +                continue;

the ! is wrong, no? otherwise you only run the code below when the path is empty, which sounds odd

> compilerprovider.cpp:168
> +                continue;
> +            } else  {
> +                //we need to search, sdk compiler names are weird: arm-linux-androideabi-g++

no need for the else + indentation, you continue above

> custommakemanager.cpp:329
> +{
> +    return KDevelop::Path(QFile::encodeName(qgetenv("CXX")));
> +}

this should not return anything and let the IADM figure out a sane default

and we should probably add a TODO to expand the custom make manager to allow setting the compiler for a given target

> qmakemanager.cpp:519
> +{
> +    return KDevelop::Path(QFile::encodeName(qgetenv("CXX")));
> +}

this is wrong, it should query the item's cache for the QMAKE_CXX variable

> qmakemanager.h:86
> +
> +    KDevelop::Path compiler(KDevelop::ProjectTargetItem * p) const override;
>      //END IBuildSystemManager

style: space before *

REPOSITORY
  R32 KDevelop

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

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