kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

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.