Hi Tobias, On Tue, Oct 14, 2025 at 7:57 PM Tobias Burnus <[email protected]> wrote: > first, sorry for the belated reply. Besides other work, I blame the > Cauldron -> https://gcc.gnu.org/wiki/cauldron2025 , > an internal company gathering, and the (virtual) OpenMP > specification meeting in the last week for not having done the > review. >
Thanks so much for the information about Cauldron! I took a look at the talks and found several interesting topics, especially your session on Parallel Computing and Richard's beginner guide to GCC vectorizers. I'll be sure to watch them! > Can you remind me (aka as 'ping' me) in a while for the two pending > sinpi patches? I first need to review some other patches first plus > make a bit progress on my own - but don't want that they fall through > the cracks. > No need to rush on my account. I'm totally okay with doing this one patch at a time - I understand you're busy and want to respect your free time. When you get a moment, I suggest we review the libquadmath patch first. Because the implementation strategy is very similar, I think it will be easier to move along quickly. This would also help projects like Flang that utilize libquadmath. Latest patch: https://inbox.sourceware.org/fortran/kl1pr0601mb4291e1457dc09fe3aa6652c884...@kl1pr0601mb4291.apcprd06.prod.outlook.com/#t > > > On Mon, Sep 22, 2025 at 3:47 PM Tobias Burnus<[email protected]> wrote: > >> Thanks for the updated patch. Glancing at the code, I wondered about: > >> > >> + if (expr->ts.type == BT_CHARACTER && !gfc_is_proc_ptr_comp (expr)) > >> > >> Namely: Why are procedure-pointer components handled differently? > > I must admit that I borrowed this code snippet from gfc_conv_variable. > > I am unsure if gfc_is_proc_ptr_comp is appropriate here, so I included > > it to keep them consistent. > > Well, this does not explain why it is there forgfc_conv_variable, either. I > thought that I should investigate, added a > gfc_warning and run the testsuite. Result: It triggers for testcases > that have the following pattern: type t procedure(deferred_len), > pointer, nopass :: ppt end type t contains function deferred_len() > character(len=:), allocatable :: deferred_len ... type(t) :: x x%ppt => > deferred_len Here, the LHS is procedure pointer that is a component of a > derived type. Before the code in question, we have at tree level: > *(x.ppt) such that we need to remove the '*' via gfc_build_addr_expr → > 'x.ppt'. * * * That was for convert_varable; however, early in > convert_constant, there is: if (expr->expr_type != EXPR_CONSTANT) ... > return; And I frankly fail how we could ever end up with a constant that > is an procedure pointer. BTW: I tried the following but it calls > convert_variable; I tried something else, but run into other issues → > https://gcc.gnu.org/PR122278 ----------------------- implicit none > (type, external) type t procedure(deferred_len), pointer, nopass :: ppt > => null() end type t type(t), parameter :: pp = t() > procedure(deferred_len), pointer :: x x => pp%ppt contains function > deferred_len() character(len=:), allocatable :: deferred_len end end > ---------------------- Thus, I wonder whether we should remove is as > never-true condition (or with '!' always true). * * * > IIUC are you suggesting we should remove the unexplainable gfc_is_proc_ptr_comp from the current patch? If so, I'm okay with that, and I will investigate the other cases you mentioned. > * * * > > > + /* TODO: support array for conditional expressions */ > > Shouldn't this be, e.g., > - TODO: support arrays in conditional expressions > - TODO: conditional expressions with arrays > - TODO: array support for/in conditional expressions > ? 'support array' sounds as if you want to add an auxiliary > array for handling conditional expressions. > > Not that it really matters as we hopefully can soon have some > array support and it is an internal comment, only :-) > > * * * > > Otherwise, LGTM. > > Thanks for the patch! > > Tobias > > PS: I was a bit unsure whether the string-length evaluation > properly handles se.pre/se.post, but it does: > > D.4681 = x != 0B; > if (D.4681) > { > D.4682 = f (); > D.4683 = *D.4682; > __builtin_free ((void *) D.4682); > ... > ..., D.4681 ? MAX_EXPR< ... D.4684>, 0> : 3 > > for > > implicit none (type, external) > integer, allocatable :: x > > print *, (allocated (x) ? "abcded"(1:f()) : "bar") > contains > function f() > integer, allocatable :: f > f = x > end > end > I will update the array comment and investigate the string length case this weekend, and then attach the updated and rebased patch. * * * Fun fact: I recently made my first commit to libstdc++, where I implemented a small C++26 paper : ) Yuao
