kicad-developers team mailing list archive

[Matthew Brigdan <mbrigdan@xxxxxxxxx>]

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?


I'm not a c++ expert by any means, but I think you might
misunderstanding what the const keyword means in this context.

A const member function in c++ is guaranteed not to alter the object
it belongs to when called. It does not have to return a constant
value, or anything like that. As far as I can tell, there is not
"casting away" of constness

In your example, when frobble_track() gets called, it is not a const
function, so the compiler knows that the object may have been
modified.

---
Matthew Brigdan