D9290: [filewidgets] Fix create path

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Thanks!

INLINE COMMENTS

> helpers.h:23
> +
> +#ifndef KIO_HELPERS_H
> +#define KIO_HELPERS_H

This file should be called *_p.h to make it clear that it's not installed (and therefore not public API).

helpers is too generic though, let's at least call this something like pathhelpers_p.h

> helpers.h:30
> +inline
> +QString addPath(QString path)
> +{

const QString & to avoid one copy

I think the template below could also take const QString &T as input.

I'm also wondering about the name. The name addPath made sense in KUrl, but here we're not "adding one path to an object". This is more about concatenating paths. How about concatPaths() ?

> helpers.h:35
> +
> +template <class T, class ...T1,
> +          class = typename std::enable_if<std::is_same<T, QString>::value>::type>

Fancy variadic template, but is it actually called with more than 2 args anywhere? :-)

If not I'd suggest to keep it a simple function with 2 args.

> helpers.h:36
> +template <class T, class ...T1,
> +          class = typename std::enable_if<std::is_same<T, QString>::value>::type>
> +T addPath(T path, T1... pathN)

is the enable_if needed? After all if someone calls this with int or whatever else, it will simply fail to compile, right?

REPOSITORY
  R241 KIO

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

To: anthonyfieroni, #frameworks, dfaure, hein, aacid
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171212/ee814ad2/attachment.html>