D5188: [WIP] Basic support for building Go projects.

[Milian Wolff]

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

INLINE COMMENTS

> gobuilder.cpp:20
> +{
> +    return createJobForAction(dom, "build");
> +}

here and below: wrap in QStringLiteral

> gobuilder.cpp:44
> +
> +#include "gobuilder.moc"

shouldn't be required

> gobuilder.h:24
> +public:
> +    KJob* build(KDevelop::ProjectBaseItem *dom) override;
> +    KJob* clean(KDevelop::ProjectBaseItem *dom) override;

why do you call these arguments `dom`?

> gobuildsystem.cpp:50
> +{
> +    return Path::List();
> +}

`return {}`

> gobuildsystem.cpp:55
> +{
> +    return Path::List();
> +}

`return {}`

> gobuildsystem.cpp:60
> +{
> +    return QHash<QString,QString>();
> +}

`return {}`

> gobuildsystem.cpp:97
> +{
> +    ProjectFolderItem *fi = dynamic_cast<ProjectFolderItem*>(item);
> +    for(; !fi && item; )

this looks like it should be a do-while

  auto project = item->project(); // remember for below
  ProjectFolderItem *folder = nullptr;
  do {
      folder = dynamic_cast<...>(item);
      item = item->parent();
  } while (!folder && item);
  
  if (folder) {
      return folder->path();
  } else {
      return project->path();
  }

> gobuildsystem.cpp:104
> +    if(!fi) {
> +        return item->project()->path();
> +    }

this can crash. note how your loop above theoretically can run until both item and fi are null, then you'd go into this branch and crash

> gobuildsystem.cpp:111
> +{
> +    return QList<ProjectTargetItem*>();
> +}

`return {}`

REPOSITORY
  R59 KDevelop Go

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

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