extragear/multimedia/amarok/src

[Jeff Mitchell]

Ian Monroe wrote:
> 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.

No, it's not ignored...trying to modify the object without const casting
it will result in a compile error.

For that reason, it's not misleading either.  The const is a warning to
the caller that they should not be modifying that object, and since the
compiler will force this upon them, it's far from misleading; it's very
direct and specific.

--Jeff