kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

On 05/26/2016 04:51 PM, Wayne Stambaugh wrote:
> On 5/26/2016 10:38 AM, Maciej Sumiński wrote:
>> On 05/26/2016 09:46 AM, jp charras wrote:
>>> Le 26/05/2016 à 08:59, Maciej Sumiński a écrit :
>>>> Thank you Cirilo. I am trying to be really careful here, that is why I
>>>> am asking for more details. I would like to make the code simpler, so
>>>> there are no more doubts about whether to use operator = or Copy() (for
>>>> some classes it is exactly the same code).
>>>>
>>>> For the scenario you described, there is EDA_ITEM::Clone() method.
>>>> Copy() is defined only for a few classes, and others are copied using
>>>> operator=, so it is a bit confusing to me.
>>>>
>>>> Regards,
>>>> Orson
>>>
>>> Usually, Copy() is used when you do not want to copy all members.
>>> Typically when you do not want to change Pback, Pnext, m_Parent, time stamp and a few other pointers
>>> and flags.
>>>
>>> If these members are reinitialized (or have the right value) after copy, using Copy() or = (for
>>> classes which do not define Copy() and can be copied by = ) can give the same result.
>>
>> Hi Jean-Pierre,
>>
>> Thank you for the explanations, it is quite clear now. The code is
>> currently a bit incoherent (e.g. ZONE_CONTAINER::Copy() or
>> TEXT_PCB::Copy()), I presume you will not mind if I fix it?
>>
>> Regards,
>> Orson
> 
> Hey Orson,
> 
> Be extremely careful here.  Getting the various copy functions wrong can
> break things in subtle ways that are difficult to debug.  Also be really
> careful with Clone() as it is used by the boost pointer containers.
> Take a look at the comment near the end of include/base_struct.h and the
> Boost pointer container documentation for how this all works.  Of course
> you could always convert the boost pointer containers to c++ containers
> using one of the pointer management objects but that would not be a
> trivial task.
> 
> That being said, I don't have any issues with using the assignment
> operator for copying objects.  It certainly would make the code easier
> to understand.  It will most likely be a big effort to implement this
> because you will have to find all of the places in the code where Copy()
> is used and change it to use the assignment operator copying properly.
> Changing this incrementally will most likely not be possible.
> 
> Cheers,
> 
> Wayne

Hi Wayne,

Welcome back!

I realize it is a very delicate code to modify and it is likely there
are parts of code that rely on certain behaviors (e.g. some Copy()
methods copying parent field or setting a new timestamp while others
not). That is why I want to fully understand the intentions and correct
usage. I believe it will be easier for other developers when all the
functions behave in a foreseeable way.

I am not going to perform this open-heart surgery on the master branch
right now. It will stay in my testing branch for some time until I feel
confident with the changes (and a few valgrind sessions).

Btw. gcc 4.8+ offers -fsanitize=address parameter which may help
squashing some subtle bugs. The price is you get crashes in places when
a program would operate on invalid data and move on, but the reward is
you quickly find faulty code fragments. Besides, it it is not as slow as
valgrind, so it does not hurt this much to have it enabled on every run.

Regards,
Orson

Attachment: signature.asc
Description: OpenPGP digital signature