[Cogl] [RFC] wip/cogl-2.0

[Robert Bragg]

On Tue, Apr 17, 2012 at 1:44 PM, Neil Roberts <neil at linux.intel.com> wrote:
> Here's some minor feedback. Other than that the patches look good to
> land to me.
>
> === 88f0ac9 Add _COGL_STATIC_ASSERT macro
>
> +_COGL_STATIC_ASSERT (sizeof (unsigned long) <= sizeof (void *),
>
> Hrmm.. this assertion is backwards, isn't it? It's not a problem with
> the patch though so I could make a separate patch to fix it unless you
> want to sneak in the fix and help me hide my mistake :)

Your comment says it's checking that an unsigned long can be stored
safely in a pointer not that pointer can be stored in an unsigned long
so it seems ok as is to me.

>
> (in cogl-util.h)
>
> +#include "config.h"
>
> I don't think it's expected to include config.h from a header. I think
> the assumption is that every C file has to include it and everything
> will break if it doesn't. config.h doesn't have any include guards so
> this will end up defining all of the macros twice. GCC seems to let you
> get away with this if you refedefine a macro to the same value but I
> don't know whether that's part of the C standard.

Ah I hadn't realized the header isn't guarded.

Since this is an internal header I suppose we can just assume that
config.h has already been included for us and maybe just for good
measure I could add something like this to util.h just to be doubly
sure config.h has been included:

#ifndef GETTEXT_PACKAGE
#error "config.h needs to be included before including cogl-util.h"
#endif

>
> === e8b4e25 Removes all remaining use of CoglHandle
>
> cogl-glsl-shader-boilerplate.h was renamed to the top level of the
> source tree instead of in cogl/. Is that deliberate? It also hasn't been
> renamed in the Makefile so presumably it will break the dist.

Oops. At one point I did get in a tangle with git because I'd removed
the boilerplate file (though not committed the change) and then when I
wanted it back I couldn't seem to reset the change or checkout the
file again. I've now moved this under cogl/ locally and updated
Makefile.am.

>
> === 266603a Switch use of primitive glib types to c99 equivalents
>
> This seems to break building when GObject introspection is enabled.
> Apparently it doesn't understand the C99 types so it can't work out what
> type CoglAngle is. If we add something like this then it works:
>
> #ifdef __GI_SCANNER__
> #define int32_t gint32
> #endif
>
> Maybe we could just leave it anyway though and if anyone complains we
> could tell them to file a bug against G-I.

Yeah I'm inclined to leave this for now. Patching G-I to understand
the c99 types should be pretty trivial and seems like the right thing
to do here. It would seem overly pedantic to me for G-I to require the
use of those typedefs over more standard types. If really necessary
then yeah later we can look at using a trick like you described.

regards,
- Robert