Re: patch for fortran-experiments branch

[Bernhard Fischer <rep dot dot dot nop at gmail dot com>]

On Wed, Jan 10, 2007 at 09:13:19PM -0800, Brooks Moses wrote:
>I know that patches to the main trunk are to be cc'ed to the 
>gcc-patches@ list.  My impression that this was also true for patches to 
>branches; is that not the case?
>
>Do we have a ChangeLog.fortran-experiments or something along those 
>lines in the tree?  (I'm writing this offline and don't have a local 
>copy, so I can't check.)  If so, it would be useful to include the 
>ChangeLog entries with posted patches.  If not, we definitely should 
>create one and backfill the existing commits into it.
>
>Also, a few style comments/nitpicks on the patch:
>
>Christopher D. Rickett wrote:
>>Index: gcc/fortran/symbol.c
>>===================================================================
>>--- gcc/fortran/symbol.c        (revision 120661)
>>+++ gcc/fortran/symbol.c        (working copy)
>>@@ -4140,6 +4140,22 @@ generate_isocbinding_symbol (const char 
>> 
>>         /* make it use associated (iso_c_binding module) */
>>         tmp_sym->attr.use_assoc = 1;
>>+
>>+        /* need to decide whether to generate the symbols for c_null_ptr
>>+         * or c_null_funptr (i.e., whether c_ptr and c_funptr are 
>>defined).
>>+         */
>
>This should be:
>
>	/* Need to decide whether to generate the symbols for c_null_ptr
>	   or c_null_funptr (i.e., whether c_ptr and c_funprt are
>	   defined).  */
>
>(That is, start the sentence with a capital letter, and don't put the 
>closing */ on a line by itself.)
>
>>+        if (s == ISOCBINDING_PTR)
>>+        {
>
>The { should be indented two characters from the if, and then the 
>enclosed text is indented an additional two characters.  (Also, note 
>that indentations of eight characters should be a tab rather than eight 
>spaces, and ten is a tab and two spaces, and so on.)
>
>>+          const char *ptr_kinds[2] = {"c_ptr", NULL};
>>+          /* c_ptr was created so create c_null_ptr */
>>+          gen_special_c_interop_ptrs(ptr_kinds, mod_name);
>
>Here, I think there should be a blank line between the comment and the 
>previous line of code.  Alternately, since it looks like the comment is 
>really explaining both lines, it should be before both of them.

Also a blank between function name and brace is missing. The coding
style mandates "func<space>()"
>
>>+        }/* end if(c_ptr created) */
>
>I think that end if comments like this are not the usual for GCC code. 
>Also, I find these more confusing than helpful, since they don't match 
>the opening "if" clause, and this one isn't the end of the if statement 
>because there's a following "else" clause.
>
>>+        else
>>+        {
>>+          const char *ptr_kinds[2] = {"c_funptr", NULL};
>>+          /* c_funptr was created so create c_null_funptr */
>>+          gen_special_c_interop_ptrs(ptr_kinds, mod_name);
>
>Again, blank line before the comment, or put the comment before both 
>lines of code.
and ditto the missing space between func and parm list.

cheers,
Bernhard