Hi Stephan,
> Seeing that you want to put that into udkapi, I'll have a closer look: ;)
hehe :)
> - Parameter names starting with an underscore are not valid.
Damn, and I thought you sanctified those by not objecting in the first
round ...
(I don't like the generated headers in C++ not having underscores or
parameters, so I usually try to smuggle underscores into the IDL ...)
Fixed.
> - KeySet: How is "inserting new elements into the key set is not
> supported" handled (e.g., is it specified what exception gets raised)?
XSet::insert allows for ElementExists and InvalidArgument only, so I
chose the latter.
Added to documentation.
> - KeySet.has, KeySet.remove, containsKey, containsValue, get, put,
> remove: You need to clarify how to compare the involved values for
> equality.
Why? Isn't this an implementation detail? In other words, shouldn't the
service implementing XMap specify this?
Hmm, okay, in this case your next request will be adding this to
css.container.Map :). Sadly, I don't have the slightest idea how I
should write in prose what (I think)
"css::uno::Any::operator==(css::uno::Any)" does ...
> - Values: "The map and the value collection are linked, in that changes
> in the map are reflected in the value collection, too." and "If you
> modify the map while an enumeration of values is running, the behavior
> of the enumeration is undefined." look like they are at odds with each
> other. What exactly is meant with "while an enumeration of values is
> running"?
Hmm ... That's an edge case, probably, depending on exactly the sentence
you pointed out.
Perhaps I should just state that operating on an XEnumeration object
which has been created (by XEnumerationAccess::createEnumeration)
*before* a change to the map happened yields undefined behavior. Would
that be better?
I also thought about simply requesting to invalidate (as in: will throw
a DisposedException when used) all enumerators (for keys, values, and
mapping pairs) once the map is modified. Would probably be the most
stringent requirement, but quite reasonable. What do you think?
> - getSize: Is it needed? (At least theoretically, the return type
> poses a problem that could be prevented simply by leaving that method out.)
What's the problem with the return type?
For one, I added this method for consistency with java.util.Map, which
strongly inspired the interface.
Then, I am undecided. In scenarios with a clear control over a given map
(i.e. where you have confidence no other instances tampers with it), it
might be interesting to know how many elements it contains, without
enumerating them all.
So, all in all, I'm in slight favor of keeping it.
> - containsKey, containsValue, get, put, remove: Why the
> NullPointerException? Why would an implementation choose not to support
> null references to interfaces as keys and/or values? How would
> KeySet.has and KeySet.remove handle this?
Not supporting NULL for either keys or values could probably be
justified (for a concrete implementation) by the additional
implementation effort/complexity it would cost. Imagine an
implementation which internally uses the strict weak ordering on allowed
keys (or even allowed values, if it chooses a certain less-than-O(n)
implementation of containsValue) - allowing for NULL keys (values) would
make the implementation more difficult. So the implementor might decide
that that's just not worth it for the intended use cases.
KeySet::has can only return false, since it does not allow for an
appropriate exception. Which, in some sense, is consistent with existing
XSet implementations - invalid keys cannot but treated as "not contained".
KeySet::remove does not have a problem, since that's unsupported, anyway
- so any other suitable exception can be thrown before actually checking
the passed key.
Of course, even if this perhaps convinced you :), it might still be
debatable whether "Null*Pointer*Exception" really is the right thing
here. Again, this was inspired by java.util.Map.
> - get: I would let it return Optional<any> and not raise
> NoSuchElementException, so that it can be used more naturally in
> multi-threaded scenarios.
Hmm. I don't like Optionals. In single-threaded scenarios, they
unnecessarily complicate the usage.
> - put: Better let it return Optional<any>. What exactly is meant with
> "@return [...] <NULL/> if there was no such previous association"?
In the complete wording, it is
@return
the value which was previously associated with the given key, or
<NULL/>, if there was no such previous association.
so, I had hope this is read as "no mapping existed for the given Key".
> - remove: An alternative would be to let it return Optional<any> and
> not raise NoSuchElementException (again, this might be more appropriate
> in certain multi-threaded scenarios).
Okay, we have "I would let it return Optional...", "Better let it return
Optional..." and "An alternative would be to let it return Optional...".
Can you decide for one of those? :)
> - create, createImmutable: You need to clarify what "unsupported types"
> are.
Done.
> - create: You need to clarify whether and when put on the resulting
> XMap will raise IllegalArgumentException.
Done. Please review, I also added a new paragraph about accepting value
types.
> - createImmutable: How are invalid elements in _values handled?
Now throws an IllegalArgumentException. I applied both rule sets (for
keys and values) to both create=>XMap::put and createImmutable.
Thanks & Ciao
Frank
--
Frank Schönheit StarOffice/OpenOffice.org Base
[email protected] +49 40 23 646 663 / +66663
Sun Microsystems Germany Hamburg, Nagelsweg 55
----------------------------------------------------------------------
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
