kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxxxx>]

On 9/15/2010 8:45 PM, Dick Hollenbeck wrote:
> 
>>>>> 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.
>>   
> 
> 
> No, my suggestion is no different than what you originally described. 
> 
> I would not continue, but it seems you misunderstood what I was saying. 
> Feel free to disagree with me, but in order to do that one must first
> understand what was said.
> 
> Let's not confuse ownership with having a pointer to something. 
> Ownership in my mind is the "obligation to delete".  It is not an
> exclusive license on holding a pointer to something, any number of
> LIB_ALIASes can point to a LIB_COMPONENT, which is what you described,
> and is what I am describing. If I am a CMP_LIBRARY, and I own any number
> of LIB_COMPONENTS, there is nothing keeping any number of LIB_ALIASes
> from pointing to them.  Yes I also own the LIB_ALIASes as a CMP_LIBRARY.
> 
> You still have the pointers from several LIB_ALIASes to a single
> LIB_COMPONENT, its just that there is no ownership in those pointers. 
> The LIB_COMPONENTs are owned by the largest container, namely the
> CMP_LIBRARY.
> 
> class CMP_LIBRARY
> {
>     std::map<LIB_COMPONENT*, int> registry;  
> 
>     registry owns the "key" part of the map pair,
>     the "int" is the reference count.  CMP_LIBRARY
>     destructor can delete all the keys, fulfilling
>     its ownership obligation.
>  
> 
> }
> 
> 
> lib_ptr->registry[component_ptr]--; // decrements the reference count.
> 
> 
> But to be honest, reference counting may not even be needed?
> 
> There would probably be no harm in never deleting the LIB_COMPONENT
> until the the CMP_LIBRARY destructor was called.  Who cares if some
> LIB_COMPONENTs stay instantiated that are no longer used by your
> aliases?  They are essentially invisible when the last LIB_ALIAS stops
> pointing to them.  This might make disk saving a little harder, but it
> is sort of like garbage collection in a Java Virtual Machine, save only
> the ones that still have aliases pointing to them.  This could be done
> by creating a std::set<LIB_COMPONENT*> on the stack in the save
> function, and a single sweep through all the aliases and record which
> LIB_COMPONENTS still have an alias user.  You can see an example of this
> in netform.cpp.

I see said the blind man.  I understand now where you were going with
you previous post.  I will revisit my code.

> 
> 
> Yes shared_ptr implements reference counting.  Probably a python
> programmer wanted to make C++ into a higher level language.  So what,
> maybe he should have just stuck with python?  Where is that reference
> count stored?  (I know the answer, but I wan't you to consider where it
> is.)  How much slower will it be to compile the whole eeschema program
> because of the required header file nesting, now that we intend to
> compile shared_ptr, which itself brings in about 20 header files, into
> nearly every one of the eeschema *.cpp files?  (If you take seriously my
> suggestion in the coding standard?)  LIB_ALIAS will be highly visible to
> many *.cpp files, and therefore so will shared_ptr be also, because it
> is a member field.

I agree with you on the Boost header nesting.  It is amazing anyone can
wrap their head around that.  I tried to follow the path for the
shared_ptr and gave up after the fifth or sixth header file.

Wayne

> 
> There are some items to factor into *your* decision.
> 
> 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
>