kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 04/27/2014 06:49 AM, Dick Hollenbeck wrote:
> On 04/26/2014 11:02 AM, Lorenzo Marcantonio wrote:
>> Looking around I found this:
>>
>> TRACK* Next() const { return (TRACK*) Pnext; }
>> TRACK* Back() const { return (TRACK*) Pback; }
>>
>> Sure, it works, foils the compiler and it compiles. However I feel
>> that's dangerous, especially with optimizations turned on.
>>
>> Explanation on why I think it was done that way:
>>
>> the obvious
>>
>> TRACK* Next() const { return Pnext; }
>>
>> won't compile since Pnext is a) actually an EDA_ITEM or such an b) more
>> importantly is const since it's a const member function. So the
>> 'correct' way would be something like:
>>
>> const TRACK* Next() const { return static_cast<const TRACK*>(Pnext); }
>>
>> Which is of course unacceptable since many times the returned objects
>> needs to be modified. It's not really a kicad problem, it's the C++
>> const philosophy which simply don't work with intrusive collections
>> (like the old borland pre-template ones).
>>
>> From a theoretical standpoint it's correct since modifying anything
>> attached to an object changes the object (indirectly). However the const
>> modifier gives the compiler a chance to optimize away the call. In the
>> (somewhat contrived) example:
>>
>> TRACK *tn = track->Next();
>> tn->frobble_track();
>> tn = track->Next();
>>
>> what would be the value of tn at the end *if* frobble_track does
>> something like:
>>
>> void TRACK::frobble_track()
>> {
>>     // Easily recognizable list unsplice
>>     Back()->SetNext(Next());
>> }
>>
>> The compiler sees that the *track object couldn't have been modified
>> (since Next() is const), so it can *legally* elide the whole third
>> statement. In that case the tn value would be wrong. If for some reason
>> decided to reevaluate it, then tn would get the new value.
>>
>> On the compiler's whim, depending on the optimization level and whatever
>> stays in the middle. Not good for me :D
>>
>> In my experience const casting is a good way to get trouble, unless you
>> are very cautious and put big red flags over the function. This doesn't
>> seem the case to me...
>>
>> What could we do? for the simpler function like this the best thing is
>> define two functions:
>>
>> const TRACK* Next() const { return static_cast<const TRACK*>(Pnext); }
>> TRACK* Next() { return static_cast<TRACK*>(Pnext); }
>>
>> ...the same technique used for read/write smart pointers.
>> Or simply don't use the const modifier to the member (it's only a pointer
>> access, probably you can't gain much from it). Or even declare Pback and
>> Pnext mutable (which is another can of worm in itself)
>>
>> Anyway, as of now, it's formally wrong (for now, it seems to work)
>>
>> Any ideas?
> 
> 
> This is done quite a number of classes.  You outlined two solutions and put them on the
> table.  I will not add to that.  Among the two you proposed, simply removing the const in
> the existing function is my preference.  Its less to compile and read, and easy to do with
> search and replace across the many classes.


But now that I think of it, that probably won't compile, because the const is there to
honour a wrapper function's const-ness.

I doubt the current code will ever cause bad behaviour.