extragear/multimedia/amarok/src

[Jeff Mitchell]

Erik Hovland wrote:
>> Erik Hovland wrote:
>>> SVN commit 874243 by hovland:
>>>
>>> Remove unneeded const.
>>>
>>> When there is a const at the end of a function declaration. For example:
>>> QString foo() const {}
>>>
>>> Then a const at the beginning is unneeded:
>>> const QString foo() const {}
>> This is not true.
>>
>> A const at the end of a function declaration means that no member
>> variables will be changed inside the function.
>>
>> A const at the beginning means that the returned object is const, i.e.
>> is not allowed to be modified.
>>
>> Some examples:
>>
>> Object* getObject() const { return &m_object; } //valid
>> Object * getObject() const { m_object++; return &m_object; } //illegal
>>
>> const Object* getObject() { return &m_object; } //valid
>> const Object * getObject() { m_object++; return &m_object; } //valid
>>
>> const Object * getObject() { m_object++; return &m_object; }
>> const Object* myObj = getObject(); myObj = otherObj; //illegal
>> Object* myObj = getObject(); //illegal, would need to const_cast
>>
>> See also http://developer.kde.org/~wheeler/cpp-pitfalls.html#const , do
>> a search for "Specifically all of the following cases are different"
>>
>> Please revert this patch.
> 
> To give you some context, the reason for the change was because
> the compiler actually reports a parse warning associated to these
> member functions because the front const is not necessary if the back
> const does not actually change anything in the object. This parse
> warning is not reported to the command line because it is not
> considered a syntax warning and the compiler recovers (and does the
> right thing). But it is still true. This is then picked up by the static
> analysis checker I have access to. If you look at the commit you will
> notice that I took care to not change member functions that return a
> pointer. Because this would not be right (but this is also reported as
> a compiler parse warning - it has a different fix that I am less likely to
> consider).
> 
> So if I revert the patch I am making it so that the returned object, which
> is a copy (since it is neither a reference or a pointer) is const and the
> function is then enforcing constness on an object that really has not right
> being const since the caller owns it.

(cc'ing amarok-devel since it's possibly a useful conversation for many)

I am generally in favor of removing compiler warnings.

However, despite what the compiler may or may not internally find to be
a parse warning, there is definitely a time and a place for copies of
objects to have constness forced on them (which they can always remove
with a const cast).  Const is half hints to the compiler, and half hints
to the programmer.  If the programmer thinks that, for whatever reason,
it would be a  Bad Idea(tm) to change the returned copy of the object
(even though the caller owns it) -- maybe it's a URL that should not be
modified, or a unique key that really shouldn't be changed -- they can
use const to give hints to someone else using the code that hey, really,
you shouldn't be modifying this object unless you really, really feel
the need.

This is why I'm somewhat surprised that the compiler would consider this
a parse warning.

Anyways, feel free to keep the patch.  In the end it's up to whoever
wrote that code to decide whether it makes sense or not -- some of that
is probably me, and some of it is probably Alejandro.

Specifically, I may change the MediaDeviceCache::DeviceType call back to
const, because other programmers should *not* be modifying that value.

Thanks,
Jeff