[g-a-devel]Re: gnopernicus bits ... & patch

["Draghi Puterity" <mp baum de>]

Dear Michael,

thank you for the time you spent to review the Gnopernicus code. Your
remarks are as always very competent and welcome. See my comments after MP:

Best regards,
Draghi



> Also included a load of warning fixes. It seems no-one builds
> gnopi '-Wall'. This is a really important thing to do, as many warnings
> point to critical bugs.

MP: True. Will do that. Thank you for pointing it out.

>
> Firstly, the code is covered in C++ style '//' comments, these
> are not portable, and illegal ANSI C, gcc just happens to support
> them.

MP: We will comply, but we are VERY unhappy about this.

>
> Some stylistic points [ how could I escape them ;-] it's
> generally regarded as bad style to use:
>
> Type* var;
>
> since this results in confusion:
>
> Type* var, this_is_not_a_var_star;
>
> Instead it's nicer for begginners to see:
>
> Type *var;
>

MP: True, but I usually address this problem by declaring one pointer var on
a line. For me the star on the right of the type is more readable as I can
read the declaration from left to right instead of right to left (i.e. "var
is a pointer to Type").  How about using

typedef  Type* PTYPE;
PTYPE pa, pb;     /* same as Type *pa, *pb; */ ?

Is this better? Anyway, if this is so ugly in this community, I will comply.

> Another minor nit; it's nice for methods to begin on the first
> character of a line, so people can grep for method implementations
> fast ( '^my_method' ) so:
>
> int brl_ser_exit_glib_poll ()
>
> becomes
>
> int
> brl_ser_exit_glib_poll ()

MP: It seems that my thinking was alterated by higher-level MS tools (i.e.
go to declaration / implementation of function, etc) ;-) Will try to comply
in the new code and review the existing code ASAP.

>
> In braille/libbrl/baumbrl.c I'm warned about:
>
> baumbrl.c:244: warning: zero-length format string
>
> kcix += sprintf (&baumdd.KeyCodes[kcix], "");
>
> What is this intended to do ?

MP: like it is now it looks indeed stupid, but the original ideea was to
supply a prefix for all the keys here. Eventually I decided to XMLize all
that it in the upper layer, (so instead of a prefix I now have a XML tag)
and I simply deleted the content of the prefix string :-D

>
> Anyway; my gnopernicus demo went well; thanks for the great
> work -

MP: Thank you!

oh, and have you added a build sheriff notice to HACKING /
> README yet ? it'd be great to snapshot gnopernicus.
>
> So; may I commit ?

MP: For my part, go ahead and comit the patches for brlimp.c, sercomm.c. For
the others I'd like to have first the OK from each devloper. Adi, to make it
easier for all, please gather all the comments from the romanian developers
in one mail.