coapp-developers team mailing list archive
-
coapp-developers team
-
Mailing list archive
-
Message #00583
Re: Engine - Teds initial take
On Wed, Aug 4, 2010 at 8:30 AM, Olaf van der Spek <olafvdspek@xxxxxxxxx> wrote:
> 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.
We haven't specified it. This is how I have coded in the past.
Personally I don't really care.
>
>> '\\ 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...
Is this really important? Why would we need a new line at the end of the file?
>> chip_core = NULL;
>
> Unnecessary, coapp_core_free already does this (it probably shouldn't)
> and the var goes out of scope anyway.
Just me explicitly tidying up.
> libcoapp/coapp.c:
>> #define _COMPILING_COAPP_H
>
> Shouldn't this be defined on project level?
No, it is included in every internal .c file and is responsible for
making sure that the dll_export/dll_import stuff is correctly
specified.
>> static enum coapp_error coapperr = COAPP_SUCCESS;
>
> What's this?
> Shouldn't globals have a g_ prefix?
I do not believe in hungarian notation. Also this will go away. It
was a passing thought on how to manage errors, except it isn't thread
safe. I have some other ideas.
> libcoapp/coapp.h:
>> COAPP_DB_CANNOT_OPEN
>
> Might want to put a comma after this so reordering and adding entries is easier.
Sure. As I mentioned the error handling is not really in place just
yet. This will be properly expanded with the usual song and dance when
I get to it.
> libcoapp/coapp_core.c:
>> if(coapp_db_open(&(c->db)) == COAPP_DB_FAILURE)
>
> Shouldn't this be != COAPP_DB_SUCCESS?
Yes, that is correct. However it will change when I get proper error
handling in.
>> coapp_core_free()
>
>> *core = NULL;
>> *core = NULL;
>
> Why twice?
> Standard free doesn't alter it's argument, should we?
Twice because I wrote it in mcdonalds and made a mistake :)
And you are right, free should probably not null it out, however I do
want it nulled.
> Sorry for all the nitpicking. ;)
Its quite all right, I like delicious nitpicking, it builds character.
--
Ted Bullock <tbullock@xxxxxxxxxxx>
Follow ups
References