Code review comment for lp:~smspillaz/compiz-libcompizconfig/ccs-object

> ccsObjectInit is only used by the test harness which ignores the return code,
> so the intended use is unclear. Surely a better return value would be a
> pointer to the initialised object?

Indeed, future merges will introduce proper constructors (but this is not trivial right now).

>
> ccsObjectRemoveInterface assigns pointers "interfaces" and "interface_types"
> to NULL without first freeing the memory referenced.
>
> ccsObjectAddInterface assigns pointers "interfaces" and "interface_types"
> without first freeing the memory referenced.

They use realloc, so I don't see why this is a problem ?

>
> struct _CCSObjectAllocationInterface (again, no need for the "_") has members
> realloc, malloc, calloc - these are reserved names.

ack.

>
> Actually, talking of reserved names: names that begin with an underscore and a
> capital are reserved, which is another reason for not using these names in the
> struct namespace. (I still see no point in having different names (e.g.
> _CCSObject) in the struct namespace.)

Right, we should evaluate this elsewhere

>
> The tests imply that the intended usage is to cast a pointer to some other
> structure (represented by "TestingObjectWrapper") to CCSObject* before passing
> that to ccsObjectXXX. If this is not the intended usage the tests are
> inappropriate. If it is intended there's negative value in these taking a
> pointer to the CCSObject as a parameter, they may as well take void* in the
> same way as ccsObjectInit does. It's a waste of the type-safety features of
> C.

Right, this sucks, but we're introspecting the state of the object. Mabye we can get rid of the casts and just do to->object.foo