zorba-coders team mailing list archive

[Markos Zaharioudakis <markos_za@xxxxxxxxx>]

Review: Disapprove

There are several problems:

1. The new string pool is not thread-safe (the current one is)

2. The check made in ~StringPool() must be implemented in the new pool as well

3. Most of the time, the strings that are inserted in the pool are "char *" strings (for example, during doc loading, libxml gives chrar* strings to the loader). The StringPool::insertc() method is there to optimize this case: it will avoid the creation of a zstring (and the resulting string copy) if the string is already in the pool, which is the most common case. This optimization is lost with the new pool.

4. I think there is a bug in BasicItemFactory::createAnyURI(). The AnyUriItem constructor does a destructive assignment (calls take() on the input zstring). So, if the expression *theUriPool->insert(value).first returns a reference to the zstring that is in the pool, that zstring will be cleared.

I am not against this change in principle, but at the same time I don't consider it important enough to spend much time on it. So, I would prefer if this is simply dropped.
-- 
https://code.launchpad.net/~paul-lucas/zorba/feature-unordered_map/+merge/117498
Your team Zorba Coders is subscribed to branch lp:zorba.