kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 09/13/2010 03:16 PM, Wayne Stambaugh wrote:
> On 9/13/2010 11:29 AM, Dick Hollenbeck wrote:
>   
>> On 09/13/2010 09:47 AM, Wayne Stambaugh wrote:
>>     
>>> Is there a set criteria for the Boost include files that are part of Kicad?
>>> The reason I ask is I wanted to use boost::shared_ptr to solve an issue I was
>>> having while working on the new component library code an found that there are
>>> some missing header files that prevent using boost::shared_ptr.  This was
>>> easily solved by adding the missing files to the Kicad source.  I had just
>>> assumed (mistake on my part) that we were using the full Boost header install.
>>>  Obviously that is not the case.  I just wanted to make sure there is no
>>> technical (or philosophical) reason not to include the additional headers
>>> required to use boost::shared_ptr before I make any commits.  Maybe we should
>>> just include all of the Boost headers rather than a subset even though it would
>>> add quite a bit of code to the Kicad source.  Anyone else have any thoughts on
>>> this?
>>>
>>> Wayne
>>>   
>>>       
>> I probably would never personally use shared_ptr because in my mind it
>> is slightly beyond what an average C++ programmer uses on a day to day
>> basis, and it obscures the clear notion of "object ownership".
>>
>> I have never (I am old, this is a long long time, and countless lines of
>> code) been in a position where I could not assign object ownership
>> clearly to one container over another.  If ever this became obscure, I
>> would probably backup and take another look.
>>     
> I'ld rather not get off topic but I think I may have found one as I have
> wrestled with a problem for several weeks and with at least two false starts.
> The problem stems from the current design of CMP_LIBRARY and it's ownership of
> LIB_ALIAS and LIB_COMPONENT objects and the interdependency between them.  My
> original attempt was to make CMP_LIBRARY own LIB_COMPONENT objects and
> LIB_COMPONENT own it's associated LIB_ALIAS objects.  This made both searching,
> indexing, and modifying the library and editing components much more complex
> than the current design so I abandoned it as one of my goals was to simplify
> the current design.  I finally decided that CMP_LIBRARY should only own
> LIB_ALIAS objects.  I created an addition LIB_ALIAS object for the root
> LIB_COMPONENT(more on this below).  I added a boost::shared_ptr for the
> LIB_COMPONENT pointer in each LIB_ALIAS that references it and I added code in
> the LIB_ALIAS destructor to keep the LIB_COMPONENT name up to date.  When the
> last LIB_ALIAS that references the shared LIB_COMPONENT is deleted, the
> LIB_COMPONENT object goes out of scope and is automatically deleted.  This
> makes sense to me as LIB_ALIAS objects no meaning without an associated
> LIB_COMPONENT.  It also made the code for updating the CMP_LIBRARY object very
> simple.  Access to the underlying LIB_COMPONENT object is always done though
> LIB_ALIAS::GetComponent().  I know your thinking why create an alias with the
> same name as the component?  This change will allow for future expansion, if we
> want to support custom fields in aliases for the so called heavy weight
> libraries (if I am correctly understanding what the library development folks
> are requesting).  This will probably make more sense when I submit the
> blueprint for the new component library file format which should be in the next
> week or so.  Except for the additional boost header files, the changes to the
> current BZR version are pretty minimal in keeping with my preference to making
> small incremental changes to the existing code.  I could submit a patch or
> publish my branch if you would like to take a look at it.  If you have any
> suggestions that can improve this design, I would be more than happy take a
> look at it.
>
> Wayne
>   

The relationship between objects as you describe it is elegant.   No
objections there.

I personally would not used shared_ptr myself.  I'm not saying don't use
it because I am respectful of your time and judgment.

Just looking the source code and the depth of nesting of the headers
used to pull off shared_ptr.hpp makes my head spin.


You asked for an alternative, will this work? :

In lieu of shared_ptr my approach would entail a factory method and
destruction method that would be responsible for creating LIB_COMPONENT
in the context of CMP_LIBRARY, even if this entailed overloading new and
delete on LIB_COMPONENT.  Anytime a LIB_COMPONENT is created, add its
LIB_COMPONENT* to a registry housed in the CMP_LIBRARY.  Anytime one is
deleted, remove the LIB_COMPONENT* from that registry.

Candidates for the registry container are:

A)  std::set<LIB_COMPONENT*> or

B)  std::map<LIB_COMPONENT*, int>


Then in the LIB_ALIAS destructor, with registry type A) do the reference
counting with the usage count held in a new field of the LIB_COMPONENT. 
With registry of type B) simply update the int part of the pair in the
map.  Again, registry resides in the CMP_LIBRARY.  When the count goes
to zero, remove the LIB_COMPONENT from registry, by using the factory
deletion function on it, overloaded delete or some function which is a
member of CMP_LIBRARY for this purpose.

In a member of CMP_LIBRARY, and assuming type B) registry:

CMP_LIBRARY::destroyAlias( alias )
{

    if( registry[libcomp]-- == 0 )
    {
        destroyLibComponent( component );  // removes from registry, and
deletes
    }
}


It may not be  that many more lines of code, if any.  But at least we
could see what's going on in a debugger.


Dick