kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxxxx>]

On 9/13/2010 5:16 PM, Dick Hollenbeck wrote:
> 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.

My first attempt was to let the CMP_LIBRARY own the LIB_COMPONENT
objects but it made the sorting, searching, and fetching names code more
complex because you have to get the aliases for each component.  This is
important for listing names in the library viewer and library editor
code.  So I abandoned that that idea as trading one set of complexity
for another.

> 
> Candidates for the registry container are:
> 
> A)  std::set<LIB_COMPONENT*> or
> 
> B)  std::map<LIB_COMPONENT*, int>

I like the idea using of a map as a temporary holder for LIB_ALIAS
object pointers associated with a LIB_COMPONENT.  Using std::map<
wxString, LIB_ALIAS*> would allow you to use destructor of LIB_ALIAS to
decouple it from it's associated LIB_COMPONENT before it gets deleted.
This could also provide a handy method for simplifying the library
editor code.

> 
> 
> 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
>     }
> }

If my understanding of shared_ptr is correct, referencing counting is
how they are implemented.  There is a shared_ptr::use_count() method
which would suggest that reference counting what is going on behind the
scenes. With my current implementation, I could have added reference
counting to LIB_COMPONENT but boost::shared_ptr solved that problem for
me. Generally speaking, Boost has proven to be very robust for all of
the things I have used it for.  Since Boost is a dependency for Kicad, I
figured why reinvent the wheel.  Granted, it is a template which can be
bit more obscure than actually implementing code within an object
itself.  But a well designed template can save you a lot of repetitive
coding.

> 
> 
> 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.

If this debugging becomes an issue, I can pretty quickly add reference
counting to LIB_COMPONENT.  The more I talk this through, the more I
like what I have so far.  I still need to do some testing to do but I
haven't found any issues with it yet.  If there are no problems I will
commit it in the next week or so.  When you get a chance to see the
code, it's simplicity may change your mind.  If we decide that it is too
obscure, I'm more than willing to change it as long as it does not add a
lot of complexity over what I have now.

Thanks for the input.  It's always nice to be able to bounce ideas off
of other developers to make sure your not too far out in left field.

Wayne

> 
> 
> Dick
> 
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>