[amarok] /: CollectionLocation: make associated collection pointer non-const

[Matěj Laitl]

(Kevin, sorry for resend, originally I failed and send it to you directly)

On 21. 1. 2012 Kevin Funk wrote:
> On Saturday 21 January 2012, 01:39, Matěj Laitl wrote:
> > Git commit 99a7d0b5bb541ed6400213d81cc7683935666514 by Matěj Laitl.
> > Committed on 20/01/2012 at 15:21.
> > Pushed by laitl into branch 'master'.
> > 
> > CollectionLocation: make associated collection pointer non-const
> 
> Hey there,
> 
> Disclaimer: I don't really know the code, so forgive me when I'm wrong.
> 
> I'm curious. Why does Collection::location() need to be non-const? From what
> I can see in your diff you can make it const, after all. (=> no const_cast
> needed at all)

This is because virtually all CollectionLocations now take (and should take) 
non-const pointer to its parent Collection. If Collection::location() would be 
const, "this" inside this method would be of type const Collection*.

Thus if you don't want const casts, location() should be non-const because it 
returns an object that is used to change the collection.

Also see [1] for some discussion how const correctness is dealed with in Qt.

[1] http://wiki.qt-project.org/API_Design_Principles#Constness

> Secondly, it should be named createLocation() since it is in fact allocating
> memory on the heap on every call, isn't it? That applies both to
> Collection::queryMaker and Collection::location, actually. They should be
> named createQueryMaker and createLocation respectively...

Indeed, I agree. (thanks to IDEs, renaming is much easier and const mangling)

> ...and made const to
> reflect their behavior (I refer to the factory pattern here).

createQueryMaker() should be const, because QueryMaker doesn't change the 
collection. createCollection() IMO shouldn't. Compare with 
createCapabilityInteface() which is (IMO correctly) non-cost probably because 
some capabilities are used to change the collection.

> (SqlCollection, for example, internally calls
> "QueryFactor::createQueryMaker() const" which is what I would expect).
> 
> Just my two cents. :)

Thanks for reviewing the commit, I thought of passing it though reviewboard, 
but then it looked (and still looks) as the right thing to do (tm). If this 
view is not shared with other developers, it can always be reverted.

Regards,
				Matěj