Re: [PR fortran/60091, patch] - Misleading error messages in rank-2 pointer assignment to rank-1 target
I've committed a slightly rewritten version of the error messages
to trunk as rev.269717, see attached.
Thanks for the review and the comments.
Harald
On 03/12/19 23:19, Thomas Koenig wrote:
> Hi Harald,
>
>> how about the attached version? It is quite verbose and produces
>> messages like
>>
>> Error: Expected list of 'lower-bound-expr:' or list of
>> 'lower-bound-expr:upper-bound-expr' at (1)
>
> I think this way of specifying error messages
>
> +#define BOUNDS_SPEC_LIST "list of %"
>
> ...
>
> + gfc_error ("Rank remapping requires a "
> + BOUNDS_SPEC_LIST " at %L",
> >where);
>
> will cause trouble in translation of the error messages.
>
> Could you maybe use something like
>
> + gfc_error ("Rank remapping requires "
> + lower and upper bounds at %L",
> >where);
>
> and possibly, instead of
>
> - gfc_error ("Either all or none of the upper bounds"
> - " must be specified at %L", >where);
> + gfc_error ("Rank remapping requires a "
> + BOUNDS_SPEC_LIST " at %L",
> + >where);
>return false;
>
> use
>
> " Rank remapping requires that all lower and upper bounds be specified"
>
> ?
>
> (And I am fairly certain that my versions are not the best possible
> ones...)
>
> So, either something like what you propsed (but without the #defines)
> or something like what I wrote above would be OK.
>
> Regards
>
> Thomas
Index: gcc/fortran/expr.c
===
--- gcc/fortran/expr.c (revision 269715)
+++ gcc/fortran/expr.c (working copy)
@@ -3703,6 +3703,7 @@
gfc_ref *ref;
bool is_pure, is_implicit_pure, rank_remap;
int proc_pointer;
+ bool same_rank;
lhs_attr = gfc_expr_attr (lvalue);
if (lvalue->ts.type == BT_UNKNOWN && !lhs_attr.proc_pointer)
@@ -3724,6 +3725,7 @@
proc_pointer = lvalue->symtree->n.sym->attr.proc_pointer;
rank_remap = false;
+ same_rank = lvalue->rank == rvalue->rank;
for (ref = lvalue->ref; ref; ref = ref->next)
{
if (ref->type == REF_COMPONENT)
@@ -3748,36 +3750,68 @@
lvalue->symtree->n.sym->name, >where))
return false;
- /* When bounds are given, all lbounds are necessary and either all
-or none of the upper bounds; no strides are allowed. If the
-upper bounds are present, we may do rank remapping. */
+ /* Fortran standard (e.g. F2018, 10.2.2 Pointer assignment):
+ *
+ * (C1017) If bounds-spec-list is specified, the number of
+ * bounds-specs shall equal the rank of data-pointer-object.
+ *
+ * If bounds-spec-list appears, it specifies the lower bounds.
+ *
+ * (C1018) If bounds-remapping-list is specified, the number of
+ * bounds-remappings shall equal the rank of data-pointer-object.
+ *
+ * If bounds-remapping-list appears, it specifies the upper and
+ * lower bounds of each dimension of the pointer; the pointer target
+ * shall be simply contiguous or of rank one.
+ *
+ * (C1019) If bounds-remapping-list is not specified, the ranks of
+ * data-pointer-object and data-target shall be the same.
+ *
+ * Thus when bounds are given, all lbounds are necessary and either
+ * all or none of the upper bounds; no strides are allowed. If the
+ * upper bounds are present, we may do rank remapping. */
for (dim = 0; dim < ref->u.ar.dimen; ++dim)
{
- if (!ref->u.ar.start[dim]
- || ref->u.ar.dimen_type[dim] != DIMEN_RANGE)
+ if (ref->u.ar.stride[dim])
{
- gfc_error ("Lower bound has to be present at %L",
+ gfc_error ("Stride must not be present at %L",
>where);
return false;
}
- if (ref->u.ar.stride[dim])
+ if (!same_rank && (!ref->u.ar.start[dim] ||!ref->u.ar.end[dim]))
{
- gfc_error ("Stride must not be present at %L",
->where);
+ gfc_error ("Rank remapping requires a "
+"list of % "
+"specifications at %L", >where);
return false;
}
+ if (!ref->u.ar.start[dim]
+ || ref->u.ar.dimen_type[dim] != DIMEN_RANGE)
+ {
+ gfc_error ("Expected list of % or "
+"list of % "
+"specifications at %L", >where);
+ return false;
+ }
if (dim == 0)
rank_remap = (ref->u.ar.end[dim] != NULL);
else
{
- if
Re: [PR fortran/60091, patch] - Misleading error messages in rank-2 pointer assignment to rank-1 target
> Le 13 mars 2019 à 13:39, Harald Anlauf a écrit : > > Hi Thomas, > > I am not so convinced that "plain english" messages are the way to go, > even if they appear better readable at first sight, if conciseness is > lost. Well, "Syntax error" is concise, but not really helpful! > The main reason is that there three variants of the messages, > depending on context. One of them refers to expecting either a > bounds-specification-list or a bounds-remapping-list. > > Do you prefer sth. like > > "All lower bounds and all or none of the upper bounds must be specified" This is what I expect for p(:,:)=>a and I won’t complain if "use the later for bound remapping" is added when the target is a rank 1 array. For mat(2, 6) => arr I would prefer the above error rather than "Expected bounds specification ». For p(1:,1:)=>a where a is a rank 1 target, I’ll go with "all the upper bounds must be specified for bound remapping" > or > > "Either all or none of the upper bounds must be specified at (1)" This is what I expect for p(1:3,1:)=>a with the same remark as above about bound remapping. > (which we currently print in another context where it is wrong), > > while other compilers print: > > E.g. Crayftn: > > p(1 ,2:3) => t >^ > ftn-1768 crayftn: ERROR SUB, File = ptr-remap.f90, Line = 3, Column = 5 > Invalid bounds-spec-list or bounds-remapping-list for this pointer > assignment. > > E.g. Intel: > > ptr-remap.f90(3): error #8524: The syntax of this data pointer assignment is > incorrect: either 'bound spec' or 'bound remapping' is expected in this > context. [1] > p(1 ,2:3) => t > ^ > > Pointer remapping belongs IMHO to the 'more advanced’ features Agreed > and requires > some technical insight to get it right, which is why I think the related > error messages should be more technical and concise. Here I disagree, the error message should try to tell the user what is wrong without requiring any access to the standard. > > I'll think for another day or two. > > Thanks, > Harald These defines should probably be swapped +#define BOUNDS_SPEC_LIST "list of %" +#define BOUNDS_REMAPPING_LIST "list of %" to match > R1035 bounds-spec is lower-bound-expr : > R1036 bounds-remapping is lower-bound-expr : upper-bound-exp Dominique
Aw: Re: [PR fortran/60091, patch] - Misleading error messages in rank-2 pointer assignment to rank-1 target
Hi Thomas,
I am not so convinced that "plain english" messages are the way to go,
even if they appear better readable at first sight, if conciseness is
lost. The main reason is that there three variants of the messages,
depending on context. One of them refers to expecting either a
bounds-specification-list or a bounds-remapping-list.
Do you prefer sth. like
"All lower bounds and all or none of the upper bounds must be specified"
or
"Either all or none of the upper bounds must be specified at (1)"
(which we currently print in another context where it is wrong),
while other compilers print:
E.g. Crayftn:
p(1 ,2:3) => t
^
ftn-1768 crayftn: ERROR SUB, File = ptr-remap.f90, Line = 3, Column = 5
Invalid bounds-spec-list or bounds-remapping-list for this pointer assignment.
E.g. Intel:
ptr-remap.f90(3): error #8524: The syntax of this data pointer assignment is
incorrect: either 'bound spec' or 'bound remapping' is expected in this
context. [1]
p(1 ,2:3) => t
^
Pointer remapping belongs IMHO to the 'more advanced' features and requires
some technical insight to get it right, which is why I think the related
error messages should be more technical and concise.
I'll think for another day or two.
Thanks,
Harald
> Gesendet: Dienstag, 12. März 2019 um 23:19 Uhr
> Von: "Thomas Koenig"
> An: "Harald Anlauf" , "Dominique d'Humières"
>
> Cc: gfortran , gcc-patches
> Betreff: Re: [PR fortran/60091, patch] - Misleading error messages in rank-2
> pointer assignment to rank-1 target
>
> Hi Harald,
>
> > how about the attached version? It is quite verbose and produces
> > messages like
> >
> > Error: Expected list of 'lower-bound-expr:' or list of
> > 'lower-bound-expr:upper-bound-expr' at (1)
>
> I think this way of specifying error messages
>
> +#define BOUNDS_SPEC_LIST "list of %"
>
> ...
>
> + gfc_error ("Rank remapping requires a "
> + BOUNDS_SPEC_LIST " at %L",
>>where);
>
> will cause trouble in translation of the error messages.
>
> Could you maybe use something like
>
> + gfc_error ("Rank remapping requires "
> + lower and upper bounds at %L",
>>where);
>
> and possibly, instead of
>
> - gfc_error ("Either all or none of the upper bounds"
> - " must be specified at %L", >where);
> + gfc_error ("Rank remapping requires a "
> + BOUNDS_SPEC_LIST " at %L",
> + >where);
> return false;
>
> use
>
> " Rank remapping requires that all lower and upper bounds be specified"
>
> ?
>
> (And I am fairly certain that my versions are not the best possible
> ones...)
>
> So, either something like what you propsed (but without the #defines)
> or something like what I wrote above would be OK.
>
> Regards
>
> Thomas
>
Re: [PR fortran/60091, patch] - Misleading error messages in rank-2 pointer assignment to rank-1 target
On Tue, Mar 12, 2019 at 11:19:07PM +0100, Thomas Koenig wrote:
> Hi Harald,
>
> > how about the attached version? It is quite verbose and produces
> > messages like
> >
> > Error: Expected list of 'lower-bound-expr:' or list of
> > 'lower-bound-expr:upper-bound-expr' at (1)
>
> I think this way of specifying error messages
>
> +#define BOUNDS_SPEC_LIST "list of %"
>
> ...
>
> + gfc_error ("Rank remapping requires a "
> + BOUNDS_SPEC_LIST " at %L",
>>where);
>
> will cause trouble in translation of the error messages.
>
I agree with Thomas here. We should try to make the
translation of error message as painless as possible.
--
Steve
Re: [PR fortran/60091, patch] - Misleading error messages in rank-2 pointer assignment to rank-1 target
Hi Harald,
how about the attached version? It is quite verbose and produces
messages like
Error: Expected list of 'lower-bound-expr:' or list of
'lower-bound-expr:upper-bound-expr' at (1)
I think this way of specifying error messages
+#define BOUNDS_SPEC_LIST "list of %"
...
+ gfc_error ("Rank remapping requires a "
+BOUNDS_SPEC_LIST " at %L",
>where);
will cause trouble in translation of the error messages.
Could you maybe use something like
+ gfc_error ("Rank remapping requires "
+lower and upper bounds at %L",
>where);
and possibly, instead of
- gfc_error ("Either all or none of the upper bounds"
-" must be specified at %L", >where);
+ gfc_error ("Rank remapping requires a "
+BOUNDS_SPEC_LIST " at %L",
+>where);
return false;
use
" Rank remapping requires that all lower and upper bounds be specified"
?
(And I am fairly certain that my versions are not the best possible
ones...)
So, either something like what you propsed (but without the #defines)
or something like what I wrote above would be OK.
Regards
Thomas
Re: [PR fortran/60091, patch] - Misleading error messages in rank-2 pointer assignment to rank-1 target
Hi Dominique,
how about the attached version? It is quite verbose and produces
messages like
Error: Expected list of 'lower-bound-expr:' or list of
'lower-bound-expr:upper-bound-expr' at (1)
(I did check other compilers. E.g. Intel and Oracle do print messages
using the 'legalese'. But user-friendliness does count, too.)
OK for trunk? Further comments?
Thanks,
Harald
On 03/11/19 10:22, Dominique d'Humières wrote:
> Hi Harald,
>
> The patch looks good to me (although I did not test it), however I don’t like
> the standard legalese in the error messages.
>
> IMO
>
> R1035 bounds-spec is lower-bound-expr :
> R1036 bounds-remapping is lower-bound-expr : upper-bound-exp
>
> should be rephrased in plain English.
>
> Thanks for the work.
>
> Dominique
Index: gcc/fortran/expr.c
===
--- gcc/fortran/expr.c (revision 269593)
+++ gcc/fortran/expr.c (working copy)
@@ -3703,6 +3703,7 @@
gfc_ref *ref;
bool is_pure, is_implicit_pure, rank_remap;
int proc_pointer;
+ bool same_rank;
lhs_attr = gfc_expr_attr (lvalue);
if (lvalue->ts.type == BT_UNKNOWN && !lhs_attr.proc_pointer)
@@ -3724,6 +3725,7 @@
proc_pointer = lvalue->symtree->n.sym->attr.proc_pointer;
rank_remap = false;
+ same_rank = lvalue->rank == rvalue->rank;
for (ref = lvalue->ref; ref; ref = ref->next)
{
if (ref->type == REF_COMPONENT)
@@ -3748,36 +3750,72 @@
lvalue->symtree->n.sym->name, >where))
return false;
- /* When bounds are given, all lbounds are necessary and either all
-or none of the upper bounds; no strides are allowed. If the
-upper bounds are present, we may do rank remapping. */
+ /* Fortran standard (e.g. F2018, 10.2.2 Pointer assignment):
+ *
+ * (C1017) If bounds-spec-list is specified, the number of
+ * bounds-specs shall equal the rank of data-pointer-object.
+ *
+ * If bounds-spec-list appears, it specifies the lower bounds.
+ *
+ * (C1018) If bounds-remapping-list is specified, the number of
+ * bounds-remappings shall equal the rank of data-pointer-object.
+ *
+ * If bounds-remapping-list appears, it specifies the upper and
+ * lower bounds of each dimension of the pointer; the pointer target
+ * shall be simply contiguous or of rank one.
+ *
+ * (C1019) If bounds-remapping-list is not specified, the ranks of
+ * data-pointer-object and data-target shall be the same.
+ *
+ * Thus when bounds are given, all lbounds are necessary and either
+ * all or none of the upper bounds; no strides are allowed. If the
+ * upper bounds are present, we may do rank remapping. */
+
+#define BOUNDS_SPEC_LIST "list of %"
+#define BOUNDS_REMAPPING_LIST "list of %"
+
for (dim = 0; dim < ref->u.ar.dimen; ++dim)
{
- if (!ref->u.ar.start[dim]
- || ref->u.ar.dimen_type[dim] != DIMEN_RANGE)
+ if (ref->u.ar.stride[dim])
{
- gfc_error ("Lower bound has to be present at %L",
+ gfc_error ("Stride must not be present at %L",
>where);
return false;
}
- if (ref->u.ar.stride[dim])
+ if (!same_rank && (!ref->u.ar.start[dim] ||!ref->u.ar.end[dim]))
{
- gfc_error ("Stride must not be present at %L",
+ gfc_error ("Rank remapping requires a "
+BOUNDS_SPEC_LIST " at %L",
>where);
return false;
}
+ if (!ref->u.ar.start[dim]
+ || ref->u.ar.dimen_type[dim] != DIMEN_RANGE)
+ {
+ gfc_error ("Expected " BOUNDS_REMAPPING_LIST " or "
+BOUNDS_SPEC_LIST " at %L",
+>where);
+ return false;
+ }
if (dim == 0)
rank_remap = (ref->u.ar.end[dim] != NULL);
else
{
- if ((rank_remap && !ref->u.ar.end[dim])
- || (!rank_remap && ref->u.ar.end[dim]))
+ if ((rank_remap && !ref->u.ar.end[dim]))
{
- gfc_error ("Either all or none of the upper bounds"
-" must be specified at %L", >where);
+ gfc_error ("Rank remapping requires a "
+BOUNDS_SPEC_LIST " at %L",
+>where);
return false;
}
+ if (!rank_remap && ref->u.ar.end[dim])
+ {
+ gfc_error ("Expected " BOUNDS_REMAPPING_LIST " or "
+
Re: [PR fortran/60091, patch] - Misleading error messages in rank-2 pointer assignment to rank-1 target
Hi Harald, The patch looks good to me (although I did not test it), however I don’t like the standard legalese in the error messages. IMO R1035 bounds-spec is lower-bound-expr : R1036 bounds-remapping is lower-bound-expr : upper-bound-exp should be rephrased in plain English. Thanks for the work. Dominique
[PR fortran/60091, patch] - Misleading error messages in rank-2 pointer assignment to rank-1 target
The PR rightly complains about bad error messages for invalid pointer
assignments. I've tried to adjust the logic slightly so that we now
print error messages that should explain more clearly what is wrong.
This required adjustment of 2 testcases, one of which also had an
incorrect comment.
OK for trunk?
Thanks,
Harald
2019-03-07 Harald Anlauf
PR fortran/60091
* expr.c (gfc_check_pointer_assign): Correct and improve error
messages for invalid pointer assignments.
2019-03-07 Harald Anlauf
PR fortran/60091
* gfortran.dg/pointer_remapping_3.f08: Adjust error messages.
* gfortran.dg/pointer_remapping_7.f90: Adjust error message.
Index: gcc/fortran/expr.c
===
--- gcc/fortran/expr.c (revision 269445)
+++ gcc/fortran/expr.c (working copy)
@@ -3703,6 +3703,7 @@
gfc_ref *ref;
bool is_pure, is_implicit_pure, rank_remap;
int proc_pointer;
+ bool same_rank;
lhs_attr = gfc_expr_attr (lvalue);
if (lvalue->ts.type == BT_UNKNOWN && !lhs_attr.proc_pointer)
@@ -3724,6 +3725,7 @@
proc_pointer = lvalue->symtree->n.sym->attr.proc_pointer;
rank_remap = false;
+ same_rank = lvalue->rank == rvalue->rank;
for (ref = lvalue->ref; ref; ref = ref->next)
{
if (ref->type == REF_COMPONENT)
@@ -3748,36 +3750,67 @@
lvalue->symtree->n.sym->name, >where))
return false;
- /* When bounds are given, all lbounds are necessary and either all
-or none of the upper bounds; no strides are allowed. If the
-upper bounds are present, we may do rank remapping. */
+ /* Fortran standard (e.g. F2018, 10.2.2 Pointer assignment):
+ *
+ * (C1017) If bounds-spec-list is specified, the number of
+ * bounds-specs shall equal the rank of data-pointer-object.
+ *
+ * If bounds-spec-list appears, it specifies the lower bounds.
+ *
+ * (C1018) If bounds-remapping-list is specified, the number of
+ * bounds-remappings shall equal the rank of data-pointer-object.
+ *
+ * If bounds-remapping-list appears, it specifies the upper and
+ * lower bounds of each dimension of the pointer; the pointer target
+ * shall be simply contiguous or of rank one.
+ *
+ * (C1019) If bounds-remapping-list is not specified, the ranks of
+ * data-pointer-object and data-target shall be the same.
+ *
+ * Thus when bounds are given, all lbounds are necessary and either
+ * all or none of the upper bounds; no strides are allowed. If the
+ * upper bounds are present, we may do rank remapping. */
for (dim = 0; dim < ref->u.ar.dimen; ++dim)
{
- if (!ref->u.ar.start[dim]
- || ref->u.ar.dimen_type[dim] != DIMEN_RANGE)
+ if (ref->u.ar.stride[dim])
{
- gfc_error ("Lower bound has to be present at %L",
+ gfc_error ("Stride must not be present at %L",
>where);
return false;
}
- if (ref->u.ar.stride[dim])
+ if (!same_rank && (!ref->u.ar.start[dim] ||!ref->u.ar.end[dim]))
{
- gfc_error ("Stride must not be present at %L",
+ gfc_error ("Rank remapping requires a "
+"bounds-specification-list at %L",
>where);
return false;
}
+ if (!ref->u.ar.start[dim]
+ || ref->u.ar.dimen_type[dim] != DIMEN_RANGE)
+ {
+ gfc_error ("Expected bounds-remapping-list or "
+"bounds-specification-list at %L",
+>where);
+ return false;
+ }
if (dim == 0)
rank_remap = (ref->u.ar.end[dim] != NULL);
else
{
- if ((rank_remap && !ref->u.ar.end[dim])
- || (!rank_remap && ref->u.ar.end[dim]))
+ if ((rank_remap && !ref->u.ar.end[dim]))
{
- gfc_error ("Either all or none of the upper bounds"
-" must be specified at %L", >where);
+ gfc_error ("Rank remapping requires a "
+"bounds-specification-list at %L",
+>where);
return false;
}
+ if (!rank_remap && ref->u.ar.end[dim])
+ {
+ gfc_error ("Expected bounds-remapping-list or "
+"bounds-specification-list at %L",
+>where);
+ }
}
}
