On Thu, Jan 29, 2009 at 11:38:34AM -0300, Jorge Arellano Cid wrote:
On Thu, Jan 29, 2009 at 02:33:30PM +0100, Hofmann Johannes wrote:
On Thu, Jan 29, 2009 at 09:37:31AM -0300, Jorge Arellano Cid wrote:
On Wed, Jan 28, 2009 at 10:44:11PM +0100, Hofmann Johannes wrote:
On Wed, Jan 28, 2009 at 01:06:51PM +0000, Jeremy Henty wrote:
On Tue, Jan 27, 2009 at 09:08:56AM +0100, Hofmann Johannes wrote:
On Tue, Jan 27, 2009 at 07:45:42AM +0000, Jeremy Henty wrote: > I thought the problem was that CssPropertyList::set() does not > free any existing value but I'm not sure I understand how these > classes are meant to manage their memory.
CSS values are stored in CssProperty::Value, which is a union. The fix is to free Value.strVal, whenever Value contains a strVal (and not an enum or length). To know this we could depend on the corresponding CssProperty::Name (freeing it for CSS_PROPERTY_FONT_FAMILY, but not for others), or add a Type member. The latter is cleaner and would not need more memory, as Name and Type could fit in two shorts.
So, which should it be? I favour the former as I think that if we want to keep the Value struct lean then it is nicer to do it by not adding fields rather than resorting to packing tricks. But that's only a mild preference: either seems OK.
Went with the former for now, but we will need to switch to the latter to support "inherit" or properties that can hold strings _or_ enums. Please check current tip.
These patches produce lots of warnings in the test directory:
I can't reproduce the warnings,
I'm using: gcc version 4.3.2 CFLAGS="-g -O0 -W" CXXFLAGS="-g -O0 -W" ./configure --enable-ssl
but I think I know the reason. It's changeset 9a7684395cca. Can you please check attached patch which tries to fix the underlying issue in an other way. Comments are welcome! We could also backout 9a7684395cca and leave things as they were before, but deleting memory allocated with strdup() is a bug.
Yes, that's a bug.
I think the solution is much simpler. The attached patch, applied on current tip, solves the issue. Please try it and commit if you like it. ;)
Right. It's really not worth the effort to avoid this cast. Committed! Thanks, Johannes