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