gcc -Woverloaded-virtual

[David Faure]

On Sunday 25 November 2007, Aurélien Gâteau wrote:
> David Faure wrote:
> 
> > I fixed the slotInfoMessage warning a better way (moving the slot to the
> > private class since it's unrelated to the one in the base class; I'll
> > commit tomorrow), but I didn't have a fix for addSubjob/removeSubjob so
> > I'm glad you raise this. Your post made me realize the argument-type
> > difference... the fact that addSubjob takes a KIO::Job and removeSubjob
> > takes a KJob is quite illogical. I would use KJob for both (since it's
> > virtual anyway) and I would handle that correctly in the code (calling the
> > KJob:: methods and not doing the KIO-specific stuff). I.e. allowing normal
> > KJobs as children of KIO::Jobs instead of half-forbidding like now. Can
> > you make that change?
> 
> I gave it a try, attached is the result, but I don't think it's the right
> way to go.
> 
> First it changes addSubjob() signature (KCompositeJob::addSubjob() returns a
> bool, while KIO::addSubjob() used to return nothing).

Ouch. When KCompositeJob was written the bool was added there but not in KIO.
I think that's a bug; the KIO version should return a bool too, to make the API more consistent.
I see you did that in the patch -- good.

> Second, more critical, the split of addSubjob() in two methods, one with the
> inheritMetaData parameter and the other without it is dangerous, because
> now in KIO::Job we have these two methods:
> 
>     virtual bool addSubjob(KJob*, bool inheritMetaData);       /* 1 */
>     bool addSubjob(KJob* job) { return addSubJob(job, true); } /* 2 */
> 
> But (2) is now virtual, because it matches the signature of
> KCompositeJob::addSubjob(). 

Right.   (It should be marked as such for clarity in the header, either with virtual or with /*reimp*/).

> I am afraid this will lead to problems if one is overloaded and not the other.

... which is exactly what the -Woverloaded-virtual warning will detect :-)

> A better fix IMO would be to get rid of (1), make (2) not inherit meta data
> and add a method like this:
> 
>     void mergeMetaDataFromJob(KJob* job)
> 
> But it means all code which relies on KIO::Job::addSubjob() merging meta
> data needs to be fixed.

This is out of the question IMHO. Merging was =true by default, so it should happen
by default, because it's the most likely case. If it's not done automatically people
will forget to call mergeMetaData for sure.

> Another solution is to make inheritMetaData a mandatory option, and get rid
> of the overload warning with a private "using" line. But that means adding
> the boolean to all code calling addSubjob().

Yeah I don't like this much either. In 99% of the cases the merging should happen
-and- people shouldn't have to think about it. "Don't merge" is really the 
special case...

I prefer the patch as it is.

-- 
David Faure, faure at kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).