Review Request: Review API of the new Path class

[Andreas Pakulat]

> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 92
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line92>
> >
> >     Is the string-conversion really needed? I would've hoped that most of the time we're dealing with KUrl's already.
> 
> Milian Wolff wrote:
>     Not if you operate on local files, e.g. Qt uses QString for local paths everywhere. Same for QFileInfo or IndexedString.
> 
> Andreas Pakulat wrote:
>     I guess the latter (IndexedString) is a bit of a problematic case, but for the others I'd say that whoever uses Path needs to do the conversion when it needs to access local files.
>     
>     If it turns out that we end up with tons of this:
>     
>     ... (absfilename is a QString produced here somehow)
>     Path p(KUrl::fromPath(absfilename))
>     
>     Then we can have a convenience constructor, but if thats not needed that often or a KUrl is being used in that place to get a local path for some Qt API and then later on a Path is constructed from the same thing, then I'd leave this constructor out.
> 
> Milian Wolff wrote:
>     Do you want to remove it to make clear that one should use the KUrl ctor if one already has a KUrl? I don't see what you gain by removing this constructor.
>     
>     Also note that the case you mention (i.e. KUrl::fromPath) should get its own method in Path imo, since in such cases we do not want to go through the costly KUrl and instead split the incoming QString path directly.

Unless I'm mistaken the Path constructor with QString is doing exactly the same as a fromPath() static function would do. And I'm suggesting to get rid of it to avoid people using it if they don't need it. This is purely to keep the API as small and simple as possible, don't add functions/constructors that nobody (or less than a handfull places) uses right now. In particular with generic types like strings in constructors since you need to do parsing on that and if you don't offer the constructor you don't need to do any parsing.

BTW: the reason conversion of string to url is so expensive is because thats not trivial, so I don't see how you can avoid the cost since you'd still need to at least verify the incoming string is a simple local filepath and not some remote url before splitting it up on slashes.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108196/#review24736
-----------------------------------------------------------


On Jan. 5, 2013, 10:59 a.m., Andreas Pakulat wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108196/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2013, 10:59 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Discuss API of the new Path class
> 
> 
> Diffs
> -----
> 
>   project/path.h PRE-CREATION 
>   project/path.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/108196/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andreas Pakulat
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130107/bc4ebb80/attachment.html>