kicad-developers team mailing list archive

[Lorenzo Marcantonio <l.marcantonio@xxxxxxxxxxxx>]

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?

-- 
Lorenzo Marcantonio
Logos Srl