On 10/12/06, Pete Robbins <[EMAIL PROTECTED]> wrote:
On 10/12/06, Geoffrey Winn <[EMAIL PROTECTED]> wrote: > Further to another thread that was discussing how sequences should be > maintained, I have been reviewing the code of SequenceImpl.cpp and I've > made > some changes that don't (shouldn't?) alter its behaviour but should make > it > easier to work with. > > It contains a number of C style macros that are used to define families of > methods that are functionally similar but operate on a range of data > types. > As it happens, four of these macros are only actually used once each, so > we > get all the problems associated with macros for no gain. I've expanded the > macros into inline code and also deleted the original macros which makes > the > code easier to read and debug. If anyone objects, the original version is > still in SVN. > > Along the way, I noticed some odd behaviours in the code that I think are > wrong, but I'd like to get a second opinion. > > The three methods > > bool SequenceImpl::addDataObject(const char* propertyName, > RefCountingPointer<DataObject> v) > bool SequenceImpl::addDataObject(unsigned int propertyIndex, > RefCountingPointer<DataObject> v) > bool SequenceImpl::addDataObject(const Property& p, > RefCountingPointer<DataObject> v) > > are intended to append a (property, value) pair to the end of the > sequence, > and differ only in how the property is identified. The middle one just > calls > the third more or less directly, but the other two have a significant > difference. > > The first checks whether the property being added is part of the data > object's type and if not, it checks whether the data object's type is > open. > If the type is open then the new property and value are added to the data > object. Then it calls the third of these methods. The third method checks > whether the property is many valued and if it is adds the value v to the > list of values for this property. Then it exits, without altering the > sequence. If the property is not many valued then the method scans the > sequence and as long as the property is not already in the sequence it > both > sets the property value in the data object and appends a new setting to > the > end of the sequence. Throughout the SDO APIs there should be consistency and there always seems to be 3 ways of identifying a property: 1. Property& prop - fairly straightforward 2. int propertyIndex - again validate the index and get on with it 3. char* propertyName - this could be name or XPath 1. is the one that does the work 2. should just find the Property and call 1 3. should find the property, using XPath if necessary, and then call 1. For an open type it may have to create the property before calling 1. > I think this is wrong twice. > > Once because depending on whether you specify the property by name or > object > reference then you may or may not be able to add a property to an open > content type. If you have the object reference (and I assume you mean the Property& here) then this property must already be defined... otherwise how did you locate it? Secondly, because adding an additional value to a many valued property > should result an an extra setting in the sequence. That sounds like a bug. You are right a new setting should be added to the sequence. The macros that are still in the source mean that these problems are > replicated in a good many methods. > > Unless anyone can explain this behaviour I plan to make them consistent > and > also partition the code into smaller methods that might be easier to > re-use > when we try to update a sequence as part of setting data object property > values. > > Regards, > > Geoff. > > This sounds like the right thing to do. ALL the macros in the code should be removed at some point. They are a pain to maintain/debug. Cheers, -- Pete
I've just checked in the change that eliminates the macros that were only used once. I'll also fix the bug(s) where the sequence isn't getting set Geoff.
