coapp-developers team mailing list archive

[Olaf van der Spek <olafvdspek@xxxxxxxxx>]

On Wed, Aug 4, 2010 at 7:32 AM, Ted Bullock <tbullock@xxxxxxxxxxx> wrote:
> Hi Folks,
>
> I just committed a couple hours of thought towards the coapp-engine to:
>
> http://bazaar.launchpad.net/~tbullock/coapp-engine/experiment/changes

Hi Ted,

Just a few quick comments from a scan of the code. Haven't compiled it yet:

chip/chip.c:
> if(err = coapp_core_new(&chip_core))

What does the style guide say about a space between if and (?
My preference is a space.

> '\\ No newline at end of file'

Might be a good idea to add this newline to every file. A shame VS
doesn't do this by default...

> chip_core = NULL;

Unnecessary, coapp_core_free already does this (it probably shouldn't)
and the var goes out of scope anyway.

libcoapp/coapp.c:
> #define _COMPILING_COAPP_H

Shouldn't this be defined on project level?

> static enum coapp_error coapperr = COAPP_SUCCESS;

What's this?
Shouldn't globals have a g_ prefix?

libcoapp/coapp.h:
> COAPP_DB_CANNOT_OPEN

Might want to put a comma after this so reordering and adding entries is easier.

libcoapp/coapp_core.c:
> if(coapp_db_open(&(c->db)) == COAPP_DB_FAILURE)

Shouldn't this be != COAPP_DB_SUCCESS?

> coapp_core_free()

> *core = NULL;
> *core = NULL;

Why twice?
Standard free doesn't alter it's argument, should we?

Sorry for all the nitpicking. ;)

Greetings,

Olaf