| Message ID | CAKwh3qhMdT62c0L_myYcoKsJSaYfxwtZ_ZNDfjP9A60-3xOWSQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Monday 03 October 2011 23:02:15 Janus Weil wrote: > Hi all, > > here is a patch for a rather long-standing PR. It continues my ongoing > campaign of improving the checks for "procedure characteristics" (cf. > F08 chapter 12.3), which are relevant for dummy procedures, procedure > pointer assignments, overriding of type-bound procedures, etc. > > This particular patch checks for the correct shape of array arguments, > in a manner similar to the recently added check for the string length > (PR 49638), namely via 'gfc_dep_compare_expr'. > > The hardest thing about this PR was to find out what exactly the > standard requires (cf. c.l.f. thread linked in comment #12): Only the > shape of the argument has to match (i.e. upper minus lower bound), not > the bounds themselves (no matter if the bounds are constant or not). > > I also added a FIXME, in order to remind myself of adding the same > check for function results soon. > > The patch was regtested on x86_64-unknown-linux-gnu. Ok for trunk? > The patch is basically OK. Any reason to keep this disabled? + case -2: + /* FIXME: Implement a warning for this case. + gfc_warning ("Possible shape mismatch in argument '%s'", + s1->name);*/ + break; + Mikael
Hi Mikael, >> here is a patch for a rather long-standing PR. It continues my ongoing >> campaign of improving the checks for "procedure characteristics" (cf. >> F08 chapter 12.3), which are relevant for dummy procedures, procedure >> pointer assignments, overriding of type-bound procedures, etc. >> >> This particular patch checks for the correct shape of array arguments, >> in a manner similar to the recently added check for the string length >> (PR 49638), namely via 'gfc_dep_compare_expr'. >> >> The hardest thing about this PR was to find out what exactly the >> standard requires (cf. c.l.f. thread linked in comment #12): Only the >> shape of the argument has to match (i.e. upper minus lower bound), not >> the bounds themselves (no matter if the bounds are constant or not). >> >> I also added a FIXME, in order to remind myself of adding the same >> check for function results soon. >> >> The patch was regtested on x86_64-unknown-linux-gnu. Ok for trunk? >> > > The patch is basically OK. thanks for the review! > Any reason to keep this disabled? > > + case -2: > + /* FIXME: Implement a warning for this case. > + gfc_warning ("Possible shape mismatch in argument '%s'", > + s1->name);*/ > + break; Well, in principle not. As you might have noticed, such a warning is already in effect e.g. when calling 'gfc_dep_compare_expr' from 'gfc_check_typebound_override'. The problem with 'check_dummy_characteristics' is that it does not directly throw an error, but only writes a message into a string variable and returns FAILURE to the calling procedure, which itself is responsible for throwing the error. This mechanism is used to enhance flexibility and improve the error message by prepending a string describing the context in which the error message occurs (e.g. we might see a mismatch of characteristics either in a procedure pointer assignment or when passing an actual argument for a dummy procedure, etc). The situation is further complicated by the fact that 'check_dummy_characteristics' is also called by 'gfc_compare_interfaces', which itself does not throw an error either, but again just passes a string to the calling procedure. If you have a cute idea how to elegantly introduce warnings into this mechanism, I'm all ears. Otherwise I'll just start by committing the patch as posted ... Thanks again for the feedback, Janus
>>> The patch was regtested on x86_64-unknown-linux-gnu. Ok for trunk? >>> >> The patch is basically OK. > Otherwise I'll just start by committing the > patch as posted ... Just did so (r179520). Cheers, Janus
On Tuesday 04 October 2011 19:01:50 Janus Weil wrote: > If you have a cute idea how to elegantly introduce warnings into this > mechanism, I'm all ears. I'm not sure that it qualifies as cute, but we could produce multi-line diagnostics in the same way c++ does (for template candidates for example), like: error/warning: #the error/warning note: in actual argument at <> # the context > Otherwise I'll just start by committing the > patch as posted ... Sure, no problem. Mikael
>> If you have a cute idea how to elegantly introduce warnings into this >> mechanism, I'm all ears. > I'm not sure that it qualifies as cute, but we could produce multi-line > diagnostics in the same way c++ does (for template candidates for example), > like: > error/warning: #the error/warning > note: in actual argument at <> # the context Maybe not the worst idea. The question is, how would we do this technically? Can it be handled with the existing infrastructure (gfc_error_... )? Alternatives might be: * simply throw the warning without context * have 'check_dummy_characteristics' not return SUCCESS/FAILURE, but something like SUCCESS/WARNING/ERROR and then react on this appropriately by throwing either an error or a warning (more tedious) Cheers, Janus
Index: gcc/fortran/interface.c =================================================================== --- gcc/fortran/interface.c (revision 179468) +++ gcc/fortran/interface.c (working copy) @@ -69,6 +69,7 @@ along with GCC; see the file COPYING3. If not see #include "system.h" #include "gfortran.h" #include "match.h" +#include "arith.h" /* The current_interface structure holds information about the interface currently being parsed. This structure is saved and @@ -1071,13 +1072,51 @@ check_dummy_characteristics (gfc_symbol *s1, gfc_s /* Check array shape. */ if (s1->as && s2->as) { + int i, compval; + gfc_expr *shape1, *shape2; + if (s1->as->type != s2->as->type) { snprintf (errmsg, err_len, "Shape mismatch in argument '%s'", s1->name); return FAILURE; } - /* FIXME: Check exact shape. */ + + if (s1->as->type == AS_EXPLICIT) + for (i = 0; i < s1->as->rank + s1->as->corank; i++) + { + shape1 = gfc_subtract (gfc_copy_expr (s1->as->upper[i]), + gfc_copy_expr (s1->as->lower[i])); + shape2 = gfc_subtract (gfc_copy_expr (s2->as->upper[i]), + gfc_copy_expr (s2->as->lower[i])); + compval = gfc_dep_compare_expr (shape1, shape2); + gfc_free_expr (shape1); + gfc_free_expr (shape2); + switch (compval) + { + case -1: + case 1: + case -3: + snprintf (errmsg, err_len, "Shape mismatch in dimension %i of " + "argument '%s'", i, s1->name); + return FAILURE; + + case -2: + /* FIXME: Implement a warning for this case. + gfc_warning ("Possible shape mismatch in argument '%s'", + s1->name);*/ + break; + + case 0: + break; + + default: + gfc_internal_error ("check_dummy_characteristics: Unexpected " + "result %i of gfc_dep_compare_expr", + compval); + break; + } + } } return SUCCESS; @@ -1131,6 +1170,8 @@ gfc_compare_interfaces (gfc_symbol *s1, gfc_symbol "of '%s'", name2); return 0; } + + /* FIXME: Check array bounds and string length of result. */ } if (s1->attr.pure && !s2->attr.pure)