| Message ID | 51F5154C.20604@netcologne.de |
|---|---|
| State | New |
| Headers | show |
Hello Thomas, Thomas Koenig wrote: > this patch yields an error for identical values in vector expression > Regression-tested. OK for trunk? > + { > + if (n->iterator != NULL) > + continue; > + > + en = n->expr; > + if (gfc_dep_compare_expr (ec, en) == 0) > + { > + gfc_error_now ("Elements with the same value at %L" > + " and %L in vector subscript" > + " in a variable definition" > + " context (%s)", &(ec->where), > + &(en->where), context); > + retval = false; > + > + /* Do not issue O(n**2) errors for n occurrences > + of the same value. */ > + break; > + > + } > + } Something went wrong with the indentation of the last two lines. Additionally: How about simply returning with an "return false;"? One error per vector subscript should be sufficient. In my humble opinion, having multiple errors will clutter more the output than helping the user - and in the code one can avoid "retval" which makes the code (nearly undetectably) cleaner. OK - with fixing the first item and considering the last item. Thanks for the patch! Tobias
Le 28/07/2013 14:57, Thomas Koenig a écrit : > Hello world, > > this patch yields an error for identical values in vector expression > subscripts. The algorithm is O(n**2) because > > a) It would be impossible to detect a([i,i]) otherwise > b) This is not likely to be a performance bottleneck because > people don't use large vector indices. > > (as noted by the different comments in the PR). > > Regression-tested. OK for trunk? > > Thomas > > > Index: expr.c > =================================================================== > --- expr.c (Revision 200743) > +++ expr.c (Arbeitskopie) > @@ -4922,5 +4924,54 @@ gfc_check_vardef_context (gfc_expr* e, bool pointe > } > } > > - return true; > + /* Check for same value in vector expression subscript. */ > + retval = true; > + > + if (e->rank > 0) > + for (ref = e->ref; ref != NULL; ref = ref->next) > + if (ref->type == REF_ARRAY && ref->u.ar.type == AR_SECTION) > + for (i = 0; i<e->rank; i++) > + if (ref->u.ar.dimen_type[i] == DIMEN_VECTOR) I think it should be: > for (i = 0; i < GFC_MAX_DIMENSIONS; i++) otherwise, I'm afraid it will silently disregard array references of the form array(1, (/ ... /)) I haven't tested though. Otherwise looks good. Mikael
Hi Tobias and Mikael, > Something went wrong with the indentation of the last two lines. Fixed. > Additionally: How about simply returning with an "return false;"? After some more thinking, I used the option that you suggested. We'll see if we get feedback from users who want something else, if any :-) > I think it should be: >> > for (i = 0; i < GFC_MAX_DIMENSIONS; i++) Fixed with + for (i = 0; i < GFC_MAX_DIMENSIONS + && ref->u.ar.dimen_type[i] != 0; i++) (I didn't want to loop over all dimensions unconditially). I also updated the test case to reflect this. Commit is at http://gcc.gnu.org/viewcvs?rev=201294&root=gcc&view=rev . Thanks a lot for the reviews! Thomas
Index: expr.c =================================================================== --- expr.c (Revision 200743) +++ expr.c (Arbeitskopie) @@ -4700,6 +4700,8 @@ gfc_check_vardef_context (gfc_expr* e, bool pointe bool unlimited; symbol_attribute attr; gfc_ref* ref; + bool retval; + int i; if (e->expr_type == EXPR_VARIABLE) { @@ -4922,5 +4924,54 @@ gfc_check_vardef_context (gfc_expr* e, bool pointe } } - return true; + /* Check for same value in vector expression subscript. */ + retval = true; + + if (e->rank > 0) + for (ref = e->ref; ref != NULL; ref = ref->next) + if (ref->type == REF_ARRAY && ref->u.ar.type == AR_SECTION) + for (i = 0; i<e->rank; i++) + if (ref->u.ar.dimen_type[i] == DIMEN_VECTOR) + { + gfc_expr *arr = ref->u.ar.start[i]; + if (arr->expr_type == EXPR_ARRAY) + { + gfc_constructor *c, *n; + gfc_expr *ec, *en; + + for (c = gfc_constructor_first (arr->value.constructor); + c != NULL; c = gfc_constructor_next (c)) + { + if (c == NULL || c->iterator != NULL) + continue; + + ec = c->expr; + + for (n = gfc_constructor_next (c); n != NULL; + n = gfc_constructor_next (n)) + { + if (n->iterator != NULL) + continue; + + en = n->expr; + if (gfc_dep_compare_expr (ec, en) == 0) + { + gfc_error_now ("Elements with the same value at %L" + " and %L in vector subscript" + " in a variable definition" + " context (%s)", &(ec->where), + &(en->where), context); + retval = false; + + /* Do not issue O(n**2) errors for n occurrences + of the same value. */ + break; + + } + } + } + } + } + + return retval; }
Hello world, this patch yields an error for identical values in vector expression subscripts. The algorithm is O(n**2) because a) It would be impossible to detect a([i,i]) otherwise b) This is not likely to be a performance bottleneck because people don't use large vector indices. (as noted by the different comments in the PR). Regression-tested. OK for trunk? Thomas 2013-07-28 Thomas Koenig <tkoenig@gcc.gnu.org> PR fortran/58009 * expr.c (gfc_check_vardef_context): Check for same values in vector expression subscripts. 2013-07-28 Thomas Koenig <tkoenig@gcc.gnu.org> PR fortran/58009 * gfortran.dg/vector_subsript_7.f90: New test.