Hi, If you'd allow me, I'd like to make some comments below:
----- Rodolfo García Peñas <[email protected]> a écrit : > From a4174c4e5a510c6035fa976026266aef82344715 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?"Rodolfo=20Garc=C3=ADa=20Pe=C3=B1as=20(kix)"?= <[email protected]> > Date: Fri, 20 Jul 2012 00:58:12 +0200 > Subject: [PATCH 1/3] menuparser.c code clean > > [...] > > static WMenuParser menu_parser_create_new(const char *file_name, void *file, > { > - WMenuParser parser; > + WMenuParser parser = wmalloc(sizeof(*parser)); > > - parser = wmalloc(sizeof(*parser)); I would tend to try keeping separated the declarative part from the functionnal part, that's why I would not merge this line. wmalloc is not a simple neutral constant value, it actually does lots of things so it may be better to keep it along the code? > > + /* Not inside an included file -> we have reached the end */ > if (cur_parser->parent_file == NULL) > - /* Not inside an included file -> we have reached the > end */ > return False; The comment looks a bit weird to me when outside the 'if'. It was like: - if the condition is true, I explain what I'm doing (in this case) - with the comment outside, it looks like we do something but the code does not do what it says, because of the 'if'. In this case, it would be better to include to change the comment to something less equivocal. But maybe that's just me? > /* read a token (non-spaces suite of characters) > - the result os wmalloc's, so it needs to be free'd */ > + * the result os wmalloc's, so it needs to be free'd */ Looks like I made two typos here, if you happen to re-propose a patch may I suggest you take the opportunity to fix the "os wmalloc's" into an "is wmalloc'd"? > - // else: the text was passed over so it will be counted > in the token from 'start' > - } else > + /* else: the text was passed over so it will be counted in the > token from 'start' */ > + } else { >From here, it looks like the /* else */ comment is lacking one level of >indentation. This comment is to explain what happens automatically in the >'else' case of the previous 'if', hence the original indentation, and *not* >what the next 'else' is doing, in which case I'd have placed it inside the >'else' construct, not before. Maybe an explicit 'else' with nothing inside but the comment would have been more explicit, like this: } else { /* comment */ } > @@ -580,8 +624,9 @@ static void menu_parser_condition_else(WMenuParser parser) > } > + > + /* The containing #if is false, so we continue skipping anyway */ > if ((parser->cond.depth > 1) && (parser->cond.stack[1].skip)) > - // The containing #if is false, so we continue skipping anyway > parser->cond.stack[0].skip = True; > else > parser->cond.stack[0].skip = !parser->cond.stack[0].skip; I'd make the same comment as for the previous case, the comment is a bit out of place outside the 'if', it would be better to keep it at its original place. Regards, Christophe. -- To unsubscribe, send mail to [email protected].
