Review request: kdelirc

Michael Zanetti michael_zanetti at gmx.net
Tue Apr 21 19:04:31 BST 2009


On Tuesday 21 April 2009 12:03:17 David Faure wrote:
> There is funny syntax in the Modes class (which inherits from qmap) :
>     Mode retMode = operator[](remote)[mode];
> Why not value(remote)[mode]? You could use value() to save a lot of
> contains() checks, too (currently, the lookup is done twice, contains and
> then []; in non-const methods where contains isn't checked, like add() and
> erase(), operator[] even risks doing unwanted insertions)
>
> Modes::generateNulls says:
>         if (!contains(*i) || !operator[](*i).contains(""))
> operator[](*i)[""] = Mode(*i, ""); if (!theDefaults.contains(*i))
> theDefaults[*i].isEmpty();
> The first line could use some simplificiations, but the second line is
> even wierder: it doesn't do anything? (it calls the isEmpty() method and
> discards the result, well and it creates an entry in theDefaults, as a side
> effect).
> Was this meant to be theDefaults.insert(*it, "") ?
> (well or even QString() instead of "", everywhere).
>

I have fixed it in most places now. 

> The #include <KApplication> in arguments.h is unneeded.
>

replaced this with #include <kdemacros.h>.




More information about the kde-core-devel mailing list