[cairo] Comments on cairo-scaled-glyph-7.diff

[Carl Worth]

On Mon, 29 Aug 2005 18:30:40 -0400, Owen Taylor wrote:
> OK, I won't claim to have caught everything that could be commented
> on (in a 6000+ line patch), but here's what I noticed going through it
> pretty carefully. In general, it looks in pretty good shape; I don't
> see anything in the overall structure that should cause problems
> for the Win32 backend.

I'll reply for the comments on the new cairo-cache interface, since I
am responsible for that portion of this patch.

I haven't done anything to actually implement things here, but I could
easily do so when things are at a convenient point, (perhaps right
after the patch lands).

> + * A #cairo_cache_entry_t contains both a key and a value for
> + * cairo_cache_t. User-derived types for cairo_cache_entry_t must
> + * be type-compatible with this structure (eg. they must have an
> + * unsigned long as the first parameter. The easiest way to get this
> + * is to use:
> 
> This looks like a carryover from the cairo_hash_entry_t docs, since
> we have two fields in cache_entry_t. I wouldn't make it optional, just
> always require it.

Agreed. How about this as a replacement paragraph:

* A #cairo_cache_entry_t contains both a key and a value for
* cairo_cache_t. User-derived types for cairo_cache_entry_t must
* have a cairo_cache_entry_t the first parameter. For example:

> Would it make sense to just push the entry_destroy function down into
> the hash table? It's generally been pretty useful to have the equivalent
> functionality in GHashTable.

I went back and forth on this while implementing cairo-hash and
cairo-cache. The conclusion I came to was that there is no case in
cairo that actually wants to insert a new entry into a hash table with
the same key as an existing entry. The current users are hash-backed
object create functions, and the cache code. With no need for that
functionality, it's actually nice to be able to have an assert
statement there instead to catch the error if it ever occurs.

We can certainly push the destroy function down into the hash table if
the need arises. Do you anticipate any need right now?

> gtk-doc style issue ... should be %TRUE to get proper markup. 
...
> gtk-doc style issue, should be _cairo_cache_insert() for consistency

Thanks. I often get those wrong.

> + * _cairo_cache_preserve:
> 
> Generally, having an API like this without reference counting is an
> invitation for subtle bugs where a function that preserve()'s calls
> another function that does preserve() then release().

Adding a count seems like a very good idea here.

> The names preserve() and release() aren't obviously paired to me.
> The conventional name for this functionality in gtk-land is
> freeze()/thaw(), or perhaps something explicit like 
> _cairo_cache_disable/enable_pruning()?

Freeze/thaw could work. There's definitely better pairing there. The
metaphor isn't perfect, since it's not as if the cache becomes
immutable when frozen, (it's just put into a state that can only grow
and not shrink). But there wouldn't be much use for a mode that
disabled insert, so using freeze/thaw here seems fine.

I don't like disable/enable as to me those suggest a behavior that
actually toggles as opposed to the stacking we just said we wanted to
have here.

> + * passed to cairo_cache_create. If the cache is already larger than
> + * max_size, no entries will be immediately removed, but the cache
> + * will be brought down to size at the time of the next call to
> + * _cairo_cache_insert.
> 
> Any particular reason why it doesn't free memory immediately? Just 
> simplicity of implementation?

No particular reason. I think I was just documenting what I had
implemented.  It should be quite easy to add immediate shrinking here.

> + * Remove an entry from the cache which has a key that matches @key,
> ====
> 
> @entry vs. @key

Thanks.

> ====
> + * if any (as determined by the keys_equal() function passed to
> + * _cairo_cache_create).
> + **/
> +cairo_private void
> +_cairo_cache_remove (cairo_cache_t	 *cache,
> +		     cairo_cache_entry_t *entry)
> +{
> +    cache->size -= entry->size;
> +
> +    _cairo_hash_table_remove (cache->hash_table,
> +			      (cairo_hash_entry_t *) entry);
> +
> +    if (cache->entry_destroy)
> +	cache->entry_destroy (entry);
> +}
> =====
> 
> This doesn't behave as documented. As implemented entry has to actually 
> be in the cache and be an entry not a key.

Oh, oops. Need to do a quick check of the callers here to see if any
rely on the documented behavior. If not, fixing the documentation
might be easier. Otherwise, _cairo_hash_table_remove should be
modified to return the entry being removed and the entry_destroy
callback should be called on that entry (if any) rather than on the
"key" that is passed in, (which should be renamed as such).

Thanks for the careful review.

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050829/a3538513/attachment.pgp