extragear/multimedia/amarok/src

[Ian Monroe]

On Wed, Oct 22, 2008 at 7:05 PM, Jeff Mitchell
<kde-dev at emailgoeshere.com> wrote:
> 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.
>

It makes 0 sense for a return type to be const. Return types are
copies. So the const there is just ignored, thus the warning. The
const is misleading.

Ian