Hi Stephan,
> Looks better. However, you should then drop "The map and the value
> collection are linked, in that changes in the map are reflected in the
> value collection, too."
>
> Also, the descriptions of the XEnumerationAccess base interface and
> KeySet should be changed to the clearer wording, too.
Done and done.
>> 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?
>
> Not sure whether this is easily implementable, so I would simply stick
> to undefined behavior.
It's pretty easy - my implementation already needs to care for a
modified map (I certainly did not want it to interpret "undefined" as
"allowed to crash"), so changing the current behavior to some "dispose"
is possible.
However, Andre Fischer (offline) raised concerns about multi-threading
usage of the whole interface. That is, since all three enumerators
(potentially) stop working as soon as the map is modified (e.g. by an
external instance), they become pretty useless as long as you cannot
lock the complete map while enumerating it - which you cannot.
Andre has a good point here. I think it could be solved by either
decoupling the enumerators from the map, so that createEnumeration
creates a copy of the to-be-enumerated elements. Alternatively, XMap
could provide some accessors retrieving a copy of all keys/values/mappings.
I am quite undecided which is better here. IMO it somewhat depends on
how often enumeration over all map elements is actually used - if pretty
often, copying should be made explicit, not implicit. If seldom, then
copying on enumerator creation might be bearable.
>>> - 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?
>
> What about a map that holds 2^31 elements?
I was tempted to say "who needs that ...". Following this immediately
disqualifies the get-a-copy-approach above, at least with using
sequences, since those are limited to 2^31, too.
Admittedly, the other container types (e.g. XSet) do not provide a size,
either. (They also do not state whether enumerators work on a copy.)
Sigh. Will remove. For consistency reasons.
>>> - 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.
>
> I fail to see how null interface references complicate things here.
Okay, it seems I used the wrong terminology all the time. With NULL, I
actually meant anys being VOID (which don't have a canonical place in a
strict weak ordering). Changing this in the documentation reveals that
NullPointerException is indeed the wrong exception here, so I will
subsume this under IllegalArgumentException, too.
>> 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".
>
> You meant "invalid keys cannot be treated as 'contained'"?
I meant "Invalid keys can be treated as 'not contained' only" :)
>> 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.
>
> I thought it was KeySet.insert that is (implicitly) unsupported, not
> KeySet.remove?
My fault. I silently assumed "remove" is also unsupported, though I
documented only "insert" is.
Off to implement "remove" ...
>>> - 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? :)
>
> No, the three cases are rather different.
But we certainly do not want only one or two of the three methods return
an Optional, do we?
I do not dare to introduce the Optional here, since in particular for
(known-to-be) immutable maps (which I think will happen quite often),
it's indeed overhead - in the current shape, a client could safely
"ignore" (well: handle ungracefully) NoSuchElementExceptions after
has/get, since they will not happen.
What about adding an XConcurrentMap, having
- Optional< any > getIfPresent( [in] any Key );
- Optional< any > putIfAbsent( [in] any Key, [in] any Value );
- Optional< any > replaceIfPresent( [in] any Key, [in] any Value );
- boolean replaceIfMapped( [in] any Key, [in] any OldValue,
[in] any NewValue );
>>> - create, createImmutable: You need to clarify what "unsupported types"
>>> are.
>> Done.
>
> - Null keys not being allowed should be part of the interface type list
> item, not a list item of its own.
See above - I always meant VOID when I wrote NULL.
> - There is no UNO type named "int".
Fixed.
> - For my tastes, these rules are too arbitrary for this "default
> implementation."
Can you elaborate?
The only "unreasonable restriction" I see at the moment is that certain
key types are rejected - notably those which do not have a canonical
strict weak ordering. I admit this is implementation-driven ... Would
you require this to be changed?
The only "unreasonable relaxation" I see at the moment is the widening
conversion for integer types. Again, this is also implementation-driven
(by your disliked "operator>>=( Any, <inttype> )"), but could easily be
changed. I'd be comfortable with removing this relaxation, if you
request it.
Anything more "too arbitrary"?
> - There is no "the <NULL/> value" in UNO. (Rather, for every interface
> type T, there is a null value of that type.)
But <VOID/> is a dedicated value, right?
> - "If the value type's class if" typo.
Fixed.
> - There are no UNO types with TypeClass::UNION or TypeClass::UNKNOWN.
> (com.sun.star.uno.TypeClass is in a poor state. I would not mention it
> at all in the definition of XMap/Map.)
Removed. (not from the implementation. Who knows when you will
re-introduce those :)
Thanks & Ciao
Frank
--
- Frank Schönheit, Software Engineer [email protected] -
- Sun Microsystems http://www.sun.com/staroffice -
- OpenOffice.org Base http://dba.openoffice.org -
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
