Re: [Tinycc-devel] Request push

[Thomas Preud'homme]

Le lundi 30 juin 2014, 16:49:02 jiang a écrit :
> Struct improved algorithms and add struct warnings and other warnings.

Hi Jiang,

> 
> The following can be compiled:
> struct st{int a;;;;;;;};
> 
> If you do not have advice, tomorrow pushed into the mob.

No Jiang, that's not how it works. There is several problem with this request:

1) You need to realize that we are all volunteer spending some of our free 
time to improve tcc. We don't necessarily check emails about tcc everyday and 
anyway 1 day is too short to review. Another consequence of this is that we 
don't have to answer within a given delay. We will do our best to answer but 
you just need to wait. Of course if too much time (like 2 weeks) pass without 
answer you can ping to ask if we saw your email. And finally I think in your 
case you should not commit without an approval first. Eventually, when your 
commit will start to improve we might tell you that you don't need to ask for 
a review anymore.

2) I gave you some comments on another patch. Before asking for another review 
you could maybe post a new version that fixes all my comments. It gives the 
feeling that you don't want to address the remarks and discourages people to 
review your patches.

3) Don't bundle several changes in one patch. It makes it more difficult to 
review. So you make one patch that fixes the bug you mentions and others (or 
maybe just one other, it depends how big it is) with the warnings

4) Your patch contains some formatting issues:

+                        if (v == 0 && (type1.t & VT_BTYPE) != VT_STRUCT){
+                            tcc_warning("declaration does not declare 
anything");
+                            break;
+                        }if (type_size(&type1, &align) < 0) {
+                            if ((a == TOK_STRUCT) && (type1.t & VT_ARRAY))
+                                flexible = 1;
+                            else
+                                tcc_error("field '%s' has incomplete type", 
get_tok_str(v, NULL));
                         }

There should be a space between the closing parenthesis ')' and the opening 
curly brace '{'. The second "if" should be on a new line or should be preceded 
by a "else"

5) The commit message is not very informative. For the commit with the bugfix 
you could say "Allow tcc to parse:" and then the example. See commit 
82969f045c99b4d1ef833de35117c17b326b46c0 for an example. You can also simply 
explain what bug you fix "Fix parsing of arbitrary number of semicolons" or 
something better and that brings me to the next comment.

6) You need to understand how a language is designed.

-                parse_btype(&btype, &ad);
+                if(!parse_btype(&btype, &ad)){
+                    if (tok == ';'){
+                        skip(';');
+                        continue;

Here you are saying that it's ok to have struct st {int a;;;;;} but not struct 
st {int a:10;;;;}. A better place to add such a check would be near the end of 
the function, around the "skip(';'). You need to understand why this is 
authorized. So as an exercise grab the C99 standard at [1] and tell me why 
this is authorized. Hint: start to read from section 6.7.2.1. Definitions are 
recursive, so you need to browse quite a lot to understand things. Try to tell 
me which lexical elements to follow (sorry, I don't know the proper term, 
shame on me) to create your testcase. Something like:

struct-or-union-specifier
  -> struct-or-union
  -> identifier (no need to detail here)
  -> struct-declaration-list
     -> struct-declaration

etc…

[1] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf

Best regards,

Thomas

signature.asc
Description: This is a digitally signed message part.