Hi Andre,
Am 10.07.24 um 10:45 schrieb Andre Vehreschild:
Hi Harald,
thanks for the review. I totally agree, that this patch has gotten bigger than
I expected (and wanted). But things are as they are.
About the coding style: I have worked in so many projects, that I consider a
consistent coding style luxury. I esp. do not have my own one anymore. The
formating you are seeing in my patches is the result of clang-format with the
provided parameter file in contrib/clang-format. I was happy to have a tool
to do the formatting, that I could integrate into my IDE, because previously it
was hard to mimic the GNU style. I try to get to the GNU style as good as
possible, where I consider clang-format doing garbage.
I see that clang-format has a "very specific opinion" on how to format the
lines you mentioned, but it will "correct" them any time I change them and
touch them later. I now have forbidden clang-format to touch the code lines,
but this means to add formatter specific comments. Is this ok?
yes, this is much better now! Thanks.
(I entirely rely on Emacs' formatting when working with C. Sometimes
the indentation at first may appear unexpected, but in most of these
cases I find that it helps to just use explicit parentheses to convince
Emacs. This is documented.)
About the assumed size arrays, that was a small change and is added now.
Great!
Note, the runtime part of the patch (pr96992_3p1.patch) did not change and is
therefore not updated.
Regtests ok on x86_64-pc-linux-gnu/Fedora 39. Ok for mainline?
Yes, this is OK now.
Thanks for the patch and your patience ;-)
Harald
Regards,
Andre
On Fri, 5 Jul 2024 22:10:16 +0200
Harald Anlauf <anl...@gmx.de> wrote:
Hi Andre,
Am 03.07.24 um 12:58 schrieb Andre Vehreschild:
Hi Harald,
I am sorry for the long delay, but fixing the negative stride lead from one
issue to the next. I finally got a version that does not regress. Please
have a look.
This patch has two parts:
1. The runtime library part in pr96992_3p1.patch and
2. the compiler changes in pr96992_3p2.patch.
In my branch also the two patches from Paul for pr59104 and pr102689 are
living, which might lead to small shifts during application of the patches.
NOTE, this patch adds internal packing and unpacking of class arrays
similar to the regular pack and unpack. I think this is necessary, because
the regular un-/pack does not use the vptr's _copy routine for moving data
and therefore may produce bugs.
The un-/pack_class routines are yet only used for converting a derived type
array to a class array. Extending their use when a UN-/PACK() is applied on
a class array is still to be done (as part of another PR).
Regtests fine on x86_64-pc-linux-gnu/ Fedora 39.
this is a really huge patch to review, and I am not sure that I can do
this without help from others. Paul? Anybody else?
As far as I can tell for now:
- pr96992_3p1.patch (the libgfortran part) looks good to me.
- git had some whitespace issues with pr96992_3p2.patch as attached,
but I could fix that locally and do some testing parallel to reading.
A few advance comments on the latter patch:
- my understanding is that the PR at the end of a summary line should be
like in:
Fortran: Fix rejecting class arrays of different ranks as storage
association argument [PR96992]
I was told that this helps people explicitly scanning for the PR
number in that place.
- some rewrites of logical conditions change the coding style from
what it recommended GNU coding style, and I find the more compact
way used in some places harder to grok (but that may be just me).
Example:
@@ -8850,20 +8857,24 @@ gfc_conv_array_parameter (gfc_se * se, gfc_expr
* expr, bool g77,
/* There is no need to pack and unpack the array, if it is contiguous
and not a deferred- or assumed-shape array, or if it is simply
contiguous. */
- no_pack = ((sym && sym->as
- && !sym->attr.pointer
- && sym->as->type != AS_DEFERRED
- && sym->as->type != AS_ASSUMED_RANK
- && sym->as->type != AS_ASSUMED_SHAPE)
- ||
- (ref && ref->u.ar.as
- && ref->u.ar.as->type != AS_DEFERRED
+ no_pack = false;
+ gfc_array_spec *as;
+ if (sym)
+ {
+ symbol_attribute *attr
+ = &(IS_CLASS_ARRAY (sym) ? CLASS_DATA (sym)->attr : sym->attr);
+ as = IS_CLASS_ARRAY (sym) ? CLASS_DATA (sym)->as : sym->as;
+ no_pack
+ = (as && !attr->pointer && as->type != AS_DEFERRED
+ && as->type != AS_ASSUMED_RANK && as->type != AS_ASSUMED_SHAPE);
+ }
+ if (ref && ref->u.ar.as)
+ no_pack = no_pack
+ || (ref->u.ar.as->type != AS_DEFERRED
&& ref->u.ar.as->type != AS_ASSUMED_RANK
- && ref->u.ar.as->type != AS_ASSUMED_SHAPE)
- ||
- gfc_is_simply_contiguous (expr, false, true));
-
- no_pack = contiguous && no_pack;
+ && ref->u.ar.as->type != AS_ASSUMED_SHAPE);
+ no_pack
+ = contiguous && (no_pack || gfc_is_simply_contiguous (expr, false,
true));
/* If we have an EXPR_OP or a function returning an explicit-shaped
or allocatable array, an array temporary will be generated which
I understand that this may be your personal coding style, but you
might keep in mind that reviewers have to understand the code, too...
I have not fully understood your logic when packing is now invoked.
We not only need to do it for explicit-size arrays, but also for
assumed-size. This still fails for my slightly extended testcase
(see attached) where I pass the class array via:
subroutine d4(x,n)
integer, intent(in) :: n
! class (foo), intent(inout) :: x(n) ! OK
class (foo), intent(inout) :: x(*) ! not OK
call d3(x,n) ! Simply pass assumed-size array
end subroutine d4
I am unable to point to the places in your patch where you need to
handle that in addition.
Otherwise I was unable to see any obvious, major problem with the
patch, but then I am not fluent enough in class handling in the
gfortran FE. So if e.g. Paul jumps in here within the next 72 hours,
it would be great.
So here comes the issue with the attached code variant.
After your patch, this prints as last 4 relevant lines:
full: -43 44 45 -46 47
48 -49 50
d3_1: -43 44 45
d3_2: 43 -44 -45
full: 43 -44 -45 -46 47
48 -49 50
while when switching the declaration of the dummy argument of d4:
full: -43 44 45 -46 47
48 -49 50
d3_1: -43 -46 -49
d3_2: 43 46 49
full: 43 44 45 46 47
48 49 50
The latter one is correct, the former one isn't.
Sorry for spoiling the show...
Nevertheless, thanks for your great effort so far!
Harald
Regards,
Andre
PS: @Paul I could figure my test failures with -Ox with x e { 2, 3, s } to
be caused by initialization order. I.e. a member was set only after it was
read.
[remaining part of mail removed]
--
Andre Vehreschild * Email: vehre ad gmx dot de
