Le 09/03/2014 20:47, jimmie.da...@l-3com.com a écrit :
> Comments from Mikael:
>
> #1. Please merge the two ifs; it seems they have exactly the same scope
> after the patch.
>
> #2. This comment applies to the TOUPPER thing below...
> .. so it should be put here. Also the indentation is wrong.
>
> #3.This is unnecessary.
>
> ===========================
>
> All corrected in the patch below.
> This patch is not very important. If we are in 'regressions fix only' mode,
> then this needs to sit until stage 1 happens....It is not worth any kind of
> breakage.
Alright, don't forget to ping the patch during next stage1 then.
Next review iteration:
> Index: gcc/gcc/fortran/symbol.c
> ===================================================================
> --- gcc/gcc/fortran/symbol.c (revision 208437)
> +++ gcc/gcc/fortran/symbol.c (working copy)
> @@ -3069,65 +3069,65 @@
>
> FOR_EACH_VEC_ELT (latest_undo_chgset->syms, i, p)
> {
> - if (p->gfc_new)
> - {
> - /* Symbol was new. */
> - if (p->attr.in_common && p->common_block && p->common_block->head)
> - {
> - /* If the symbol was added to any common block, it
> - needs to be removed to stop the resolver looking
> - for a (possibly) dead symbol. */
> + /* Symbol was new. Or was old and just put in common */
> + if ((p->gfc_new ||
> + (p->attr.in_common && !p->old_symbol->attr.in_common )) &&
> + p->attr.in_common && p->common_block && p->common_block->head)
write this instead:
if (p->attr.in_common && p->common_block && p->common_block->head
&& (p->gfc_new || !p->old_symbol->attr.in_common))
[p->attr.in_common is less likely than p->gfc_new which happens all the
time; we may as well short-circuit the latter.]
For the rest of the patch, there are indentation issues, any leading 8
spaces block should be replaced with a tab.
You can use "contrib/check_GNU_style.sh your_patch" to check common
style issues)
> + )
> + {
> + /* If the symbol was added to any common block, it
> + needs to be removed to stop the resolver looking
> + for a (possibly) dead symbol. */
>
> - if (p->common_block->head == p && !p->common_next)
> - {
> - gfc_symtree st, *st0;
> - st0 = find_common_symtree (p->ns->common_root,
> - p->common_block);
> - if (st0)
> - {
> - st.name = st0->name;
> - gfc_delete_bbt (&p->ns->common_root, &st,
> compare_symtree);
> - free (st0);
> - }
> - }
> + if (p->common_block->head == p && !p->common_next)
> + {
> + gfc_symtree st, *st0;
> + st0 = find_common_symtree (p->ns->common_root,
> + p->common_block);
> + if (st0)
> + {
> + st.name = st0->name;
> + gfc_delete_bbt (&p->ns->common_root, &st, compare_symtree);
> + free (st0);
> + }
> + }
>
> - if (p->common_block->head == p)
> - p->common_block->head = p->common_next;
> - else
> - {
> - gfc_symbol *cparent, *csym;
> + if (p->common_block->head == p)
I think the code block starting here is indented too much.
> + p->common_block->head = p->common_next;
> + else
> + {
> + gfc_symbol *cparent, *csym;
>
> - cparent = p->common_block->head;
> - csym = cparent->common_next;
> + cparent = p->common_block->head;
> + csym = cparent->common_next;
>
> - while (csym != p)
> - {
> - cparent = csym;
> - csym = csym->common_next;
> - }
> + while (csym != p)
> + {
> + cparent = csym;
> + csym = csym->common_next;
> + }
>
> - gcc_assert(cparent->common_next == p);
> + gcc_assert(cparent->common_next == p);
> + cparent->common_next = csym->common_next;
> + }
> + }
> + if (p->gfc_new)
Same here, the code after this shouldn't need reindenting.
> + {
> + /* The derived type is saved in the symtree with the first
> + letter capitalized; the all lower-case version to the
> + derived type contains its associated generic function. */
>
> - cparent->common_next = csym->common_next;
> - }
> - }
> + if (p->attr.flavor == FL_DERIVED)
> + gfc_delete_symtree (&p->ns->sym_root, gfc_get_string ("%c%s",
> + (char) TOUPPER ((unsigned char)
> p->name[0]),
> + &p->name[1]));
> + else
> + gfc_delete_symtree (&p->ns->sym_root, p->name);
>
> - /* The derived type is saved in the symtree with the first
> - letter capitalized; the all lower-case version to the
> - derived type contains its associated generic function. */
> - if (p->attr.flavor == FL_DERIVED)
> - gfc_delete_symtree (&p->ns->sym_root, gfc_get_string ("%c%s",
> - (char) TOUPPER ((unsigned char) p->name[0]),
> - &p->name[1]));
> - else
> - gfc_delete_symtree (&p->ns->sym_root, p->name);
> -
> - gfc_release_symbol (p);
> - }
> - else
> - restore_old_symbol (p);
> + gfc_release_symbol (p);
> + }
> + else
> + restore_old_symbol (p);
> }
> -
> latest_undo_chgset->syms.truncate (0);
> latest_undo_chgset->tbps.truncate (0);
>
>
Mikael
