[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-commits
Subject:    Re: KDE/kdelibs/kdeui/icons
From:       Marc Mutz <marc () klaralvdalens-datakonsult ! se>
Date:       2007-09-18 22:17:22
Message-ID: 200709190016.21201.marc () klaralvdalens-datakonsult ! se
[Download RAW message or body]

On Tuesday 18 September 2007 15:42, André Wöbbeking wrote:
> On Tuesday 18 September 2007, Marc Mutz wrote:
> > SVN commit 713923 by mutz:
> >
> > Constructing a KIcon must surely be one of the more often-used parts
> > in program startup. The funnier that this is prematurely pessimized
> > by passing around and assinging a QStringList that's unused in 99.99%
> > of cases. Shy away from default arguments. Esp. if you go ahead and
> > switch on them in the function body! This is BC+SC
>
> Do you've some numbers how expensive the creation or passing of a
> QStringList is?

Nope. That's why I said it's fixing a premature pessimization. No-one asks for 
numbers when you pass-by-value instead of const-reference. It's obviously 
slower, and one really doesn't care how much.

> >  M  +10 -0     kicon.cpp
> >  M  +13 -2     kicon.h
> >  M  +7 -0      kiconengine.cpp
> >  M  +5 -0      kiconengine_p.h
> >
> >
> > --- trunk/KDE/kdelibs/kdeui/icons/kicon.h #713922:713923
> > @@ -24,6 +24,7 @@
> >  #include <QtGui/QIcon>
> >
> >  class KIconLoader;
> > +class QStringList;
> >
> >  /**
> >   * \short A wrapper around QIcon that provides KDE icon features
> > @@ -50,10 +51,20 @@
> >       *                 loaded from the emblems icons and up to four
> > (one per *                 corner) is currently supported
> >       */
> > -    explicit KIcon(const QString& iconName, KIconLoader* iconLoader
> > = 0L, -                   const QStringList& overlays =
> > QStringList());
> > +    explicit KIcon(const QString& iconName,
> > KIconLoader* iconLoader,
> > +                   const QStringList& overlays);
>
> still explicit?

Yeah, keeps the ctors aligned :)
Seriously, I didn't pay attention. Doesn't hurt, though.

> >      /**
> > +     * \overload
> > +     */
> > +    explicit KIcon(const QString& iconName, KIconLoader*
> > iconLoader); +
> > +    /**
> > +     * \overload
> > +     */
> > +    explicit KIcon(const QString& iconName);
>
> Why not only one ctor:
>
>    explicit KIcon(const QString&, KIconLoader* = 0);

Because we're switching on the defaulted argument inside the function body. 
That's dumb. Why would you ever want to keep the compiler from telling you 
that an optional argument wasn't given? You'd only do that if that knowledge 
wouldn't yield any benefit.

Example:
   QStringList QString::split( const QChar & ch=QLatin1Char('\n') ) const;
\n is in no way special, the algorithm is unaffected, we do not switch on 'ch' 
in the function body, so a default argument would be ok here.
   bool QString::endsWith( const QString & s,
                 Qt::CareSensititivity cs=Qt::CaseSensitive ) const;
Here, you can be more efficient if you already know it's CaseSensitive. The 
compiler has less variables, can optimize better, yadda-yadda. However, the 
best reason is: first thing the function body does? Switch on 'cs'. Ergo, the 
default argument there is a bad idea (that doesn't mean that Qt doesn't do 
it). But the best anti-example is multi-arg. If you actually implemented that 
in one function, with defaults, you would not only spend time calculating the 
number of arguments actually passed, you couldn't even decide whether 
arg2.isNull() was the user pushing QString() in that slot, indicating that he 
wanted %2's to be replaced by the empty string, or just the default value, in 
which case the expected behaviour would be to leave the %2 alone, for the 
next .arg() to come along.

Default arguments are _not_ lazy. They require time to construct, and pass, 
and, more often than not, that's time wasted.

Thanks,
Marc
-- 
Marc Mutz -- marc@klaralvdalens-datakonsult.se, mutz@kde.org
Klarälvdalens Datakonsult AB, Platform-independent software solutions

[prev in list] [next in list] [prev in thread] [next in thread]