[Kde-accessibility] [PATCHES] First round of const correctness for the KSpeech interface

[Matt Rogers]

On Saturday 15 January 2005 11:29 am, Gary Cramblitt wrote:
> On Saturday 15 January 2005 12:47 am, Matt Rogers wrote:
> > Hi,
> >
> > Attached are two patches that start to establish const correctness for
> > the KSpeech interface in kdelibs/interfaces/kspeech. A simple explanation
> > of const correctness is that class member functions that modify member
> > variables should not be const and those that do not modify member
> > variables should be const. The patch for kdelibs also fixes issues with
> > parameter default values when passing const references ( 'const QString&'
> > for example ) and passing basic types by value that are const ( 'const
> > int' for example ). I've checked to make sure that everything compiles,
> > and the patches are pretty
> > self-explanatory.
>
> Thank you for the patches.  I've committed them.
>
> > This is just the first go at this and somebody more familiar with the
> > interface and the accompanying code that uses it should continue what
> > i've started, since they'll know more about it than I will.
>
> When time permits, I will do a review of the rest of the code for const
> correctness.  I could have used your expertise about 9 months ago. :/
>
> > I would also recommend changing the class name in
> > kdelibs/interfaces/kspeech/kspeech.h from 'kspeech' to 'KSpeech'. It
> > would then follow the naming convention used throughout KDE. A short
> > changelog of my changes is provided below.
>
> That is a historical accident.  When I took over kttsd from pupeno, that
> was the object name and a couple of apps were already using the interface. 
> I think it might be possible to change the name from "kspeech" to "KSpeech"
> and define a new object called "kspeech" that inherits the exact same
> interface as "KSpeech".  The latter would not be documented with doxygen. 
> This would clean up the apidocs and encourage everyone to use KSpeech, but
> maintain backwards compatibility to existing apps.  I wonder if there is a
> way to do this without doubling the marshaling code.
>
> > ---------
> >
> > Changes:
> >
> > Remove default values of NULL when passing const references. For one,
> > NULL is not the same as QString::null. I just realized that some
> > parameters may want a default value of QString::null but nothing barfed
> > on compile, so I don't think it's that big of an issue since the
> > implementation of the interface seems to define proper default values as
> > well.
>
> I've tested and there don't seem to be any problems so far with your
> changes.
>
> > Remove the const when passing basic types (such as int or bool) by value.
> > People will laugh at you if you do it, it's not good C++ AFAIK, and since
> > you're passing by value anyways, a copy of the data is made already and
> > you don't modify the original.
>
> Thanks.  I'm a newbie at C/C++, so this is educational for me.
>
> > All the rest of the changes are to adapt the code so that it's more const
> > correct so that it all compiles. There is one FIXME that's been added,
> > but i'm sure someone with more knowledge about the code could fix it
> > quicker than i can.
>
> That's the problem with const methods, isn't it?  Since TalkerMgr is an
> object for internal use within kttsd, I wonder if it might be simpler to
> *not* const the methods in this case, since signals would be a hassle and
> complicate things, and the goal is more understandable code?
>

Actually, i would argue that always properly defining your functions as const 
where they should be is just good coding practice, whether they're internal 
or not. basically, something like the following would be easy enough to fix 
the FIXME.

in the TalkerMgr constructor:

connect(this, SIGNAL(needsCaching(const QString&, int)),
        this, SLOT(cacheTalker(const QString&, int)));

replace the 

"const_cast<TalkerMgr*>(this)->m_talkerToPlugInCache[talker] = winner;"

with 
 
emit needsCaching(talker, winner);


Then, in the new slot:

void TalkerMgr::cacheTalker(const QString& talker, int winner)
{
	m_talkerToPlugInCache[talker] = winner;
}


> > And to think, all of this started because i wanted to eliminate doxygen
> > warnings when doing make apidox in kdelibs... :)
>
> Did you succeed in eliminating all the warnings?  I noted them months ago,
> but didn't know how to fix them.
>

I think i have. Will commit soon now that you've committed the patch i sent.

> BTW, if you *really* want to help, I need someone to look at the entire
> kttsd project from the standpoint of preparing it for proper translation. 
> I've read, googled, and asked for help on this until I'm bleary-eyed.  For
> someone who fully understands translation, shouldn't require more than an
> hour or two of work.
>
> Thanks again.

Except that I don't fully understand most of the translation mechanisms except 
for the simple i18n calls. I might take a look though if curiousity strikes 
me.

Matt