← Back to team overview

kicad-developers team mailing list archive

Re: Boost include files.


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

    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.

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.

There are some items to factor into *your* decision.


Follow ups