Review Request: Review API of the new Path class

Milian Wolff mail at milianw.de
Mon Jan 7 11:25:54 UTC 2013 


> 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.
> 
> Andreas Pakulat wrote:
>     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.

No, the Path(QString) ctor is the same as Path(KUrl(QString)) which in turn is the same as Path(KUrl::fromPathOrUrl(QString)). This is being used at quite a few places in our code, but its mostly unit tests which pass either URLs or local paths.

I'll investigate what will break if we'll remove that ctor and look at the required porting effort. Thinking about it some more I also agree that it should be "fast enough" to use KUrl::fromPath where appropriate to speed up the process versus rolling our own path parsing. Especially because I forgot that we want clean paths thus would have to implement that on our own as well.

Thanks for nagging Andreas :)


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 311
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line311>
> >
> >     Hmm, I think I'd separate the remote-url part from the path itself here. There are some places in the code where you have explicit checks for remote-urls and then take the first or second item from the list. Having a separate member would make that code a lot clearer.
> 
> Milian Wolff wrote:
>     I also thought about that but deliberately decided against it for space reasons (saving one pointer for local paths). But I agree that it might be a bit over-optimized. I'll revisit the code and take another look at it.
> 
> Andreas Pakulat wrote:
>     I see, but in this case I'd say optimization comes after code-readability. So once everythings ported we should measure the difference between saving the pointer or not and then decide wether saving it outweighs the special code in the various functions - at least IMO.
> 
> Milian Wolff wrote:
>     I did compare it back then but sadly I just realize that my results in the commits are not comparable: 949ed44e369d8d3ce798737f1a3517bc9fe48caa vs c6dca99c710f99c7ddbc47b00dd70157f7d3a076 also shows different numbers for the other datatypes... I'll redo the measurement and figure out whether it's worth it.
> 
> Milian Wolff wrote:
>     Looking at it again, I have to say that the code readability would not increase considerably imo... Especially since you'd now have to compare/check two variables instead of just one the code becomes bigger which is arguably also worse to read.
>     
>     And so far only 6 places or so need to take special care when dealing with the aggregated prefix-in-m_data. Imo this is not so bad and I'd like to keep it as-is.
> 
> Andreas Pakulat wrote:
>     So you think that this (from setFilename):
>     
>         if( m_data.isEmpty() ) {
>           m_data.append(name);
>         } else {
>           m_data.last(name);
>         }
>     
>     is not easier to understand than the current code:
>     
>         if (m_data.isEmpty() || (!isLocalFile() && m_data.size() == 1)) {
>             // append the name to empty Paths or remote Paths only containing the Path prefix
>             m_data.append(name);
>         } else {
>             // overwrite the last data member
>             m_data.last() = name;
>         }
>     
>     I wholeheartedly disagree. Or this one (from up/parent):
>     
>         if( m_data.size() == 1 ) {
>           ret.m_data.last().clear();
>         } else {
>           ret.m_data.pop_back();
>         }
>     
>     is certainly easier to understand than this one (and both are actually shorter as well):
>     
>         if (m_data.size() == (1 + (isRemote() ? 1 : 0))) {
>             // keep the root item, but clear it, otherwise we'd make the path invalid
>             // or a URL a local path
>             ret.m_data.last().clear();
>         } else {
>             ret.m_data.pop_back();
>         }
>     
>     The reason is simply that its more self-explanatory to someone who hasn't written up the code. In the current code one first has to think through why with isRemote() it compares to 2 before clearing the last element, so you always have to keep that special behaviour in the back of your head when reading or debugging the code. Can you show the example where you actually get more code for the separation of url and path (yeah I'm too lazy to look through the complete implementation)?

Hehe these will of course become simpler. I don't debate that. When I started introducing the second member yesterday, I did dislike it in a few places. I'll just do it again and show you the diff.


- Milian


-----------------------------------------------------------
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/f92ba265/attachment.html>

More information about the KDevelop-devel mailing list