On 11/12/06, Geoffrey Winn <[EMAIL PROTECTED]> wrote:
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.
The handling of sequences is a bit more complicated than I thought. There are already some places where a set operation will cause an update to a data object's sequence (if it has one) notably when reading open types from an XML document - unfortunately there are plenty more places where this isn't done. Then there are cases where a (property, value) pair have been added to a sequence, the property or value gets removed from the data object but the sequence is left as is, containing a reference to something that no longer exists. I'll start fixing these cases one at a time because I think all of this at once is bound to break something. Geoff.
