Sorry to drop out on this. I got interrupted by other duties in life, and now I am no longer able to continue this work. If anybody wants to pick up the ball from here, please feel free to commandeer the patch.

Fixed in D136479.

Fixed in D136479.

Fix test link-f90-main.f90, which was failing for statically linked builds.

CI fail noted, update will come.

In D127337#3815169, @jeanPerier wrote:

Sorry for the late feedback.

But the logic behind each of them is clear (the subexpression in question is marked for each warning). I expect that limiting to one warning per expression would complicate the logic. Would that be worth it, or even desired?

As far as I am concerned, I think the warning duplication is noise, and I think it would be worth trying to limit them.

ping

In D125804#3740598, @PeteSteinfeld wrote:

When I run "check-flang" after making these changes, I get 21 failures:

Yes, I know. The dependencies intend to make those tests conform to the stricter testing of this patch. I will rebase when the last two dependencies are merged. (The base commit is a little stale. Currently, four tests would still fail if this patch was applied on top of main.)

ping.

In D130386#3727156, @klausler wrote:

In D130386#3726982, @ekieri wrote:

I am sorry the warning patch (D125804) is taking so long, I have not found much time to work with Flang lately. This patch causes a mismatch between emitted and expected warnings in associated.f90, line 141. Could you please have a look to see if the changed warning message is deliberate or not?

A warning is appropriate there.

I am sorry the warning patch (D125804) is taking so long, I have not found much time to work with Flang lately. This patch causes a mismatch between emitted and expected warnings in associated.f90, line 141. Could you please have a look to see if the changed warning message is deliberate or not?

Thanks!

I think you can use the WARNING-directives without waiting for D125804. It will take me some time to land it, and I do not think it should block other developments. In the meantime, it would only be helpful if WARNING-directives are included in new (and existing) tests, like you did here. It would, for now, give a false impression of the warnings being tested, but at least they will be in the future (and if it regresses before then, I will notice). Refraining from testing warnings in the meantime comes with the risk of forgetting to add the tests later.

I post these in a separate patch for the extra visibility, as I would like some input. What do you make of this? resolve37.f90, with the four consecutive WARNING-directives, looks a bit silly, and the repetition is not particularly helpful. But the logic behind each of them is clear (the subexpression in question is marked for each warning). I expect that limiting to one warning per expression would complicate the logic. Would that be worth it, or even desired? Note that integer division-by-zero in a constant expression also triggers an error.

By the way, did you note that the CI has some trouble with the formatting?

In D126890#3567559, @awarzynski wrote:

Thanks, can I assume that you are OK with this change?

In D126890#3565592, @awarzynski wrote:

Both tools drive the same set of libraries, but these libraries are very complex (i.e. require a lot of set-up that depends on user input) and the drivers are almost completely separate. It is hard to guarantee that the behavior is consistent without testing with both (in fact, we did see them diverge in the past - that happens when updates are made in one tool, but not in the other).

Is it the right trade-off between test coverage and computational effort to have dual run lines for the tco cases? I would assume that in most cases, what is actually tested will be deferred to the same code by both tools. Could we get almost the same coverage for half the effort by using the one tool for half the tests and the other for the rest, or would we miss something important by doing that?

Sure, if you find my draft message useful, please go ahead. Whether you use it or not, consider me satisfied. But yes, please wait for Steve.

Thanks for doing this, would be great if we can reduce the confusion generated by this script. I have almost never used this script myself, and is not really sure what it is for, so I am perhaps not in the best position to comment. But here are my thoughts, for what it is worth:

Looks good, thanks!

LGTM, just a small suggestion: Could you add a note about the missing tests (that (and why) they are in D124667) to the commit message?

Thanks for addressing my comments!

Thanks for taking the time to work on quality and compliance! LGTM, but please consider my nits.

One nit/question inline, otherwise LGTM.

Ok, good. Then please abandon this revision (Add Action... down by the comment box), so that its status is clear to everybody. Cheers!

Hi! Thanks for this patch, and for your interest in contributing to Flang!

Thank you all for the support!

This patch is the result of discussions in https://reviews.llvm.org/D122542

Address review comment.
Fix typo in summary.

Thanks for the suggestion, Pete!

Hi Andrzej, many thanks for all the encouragement! Yes, I added you more
like a cc (could rather have used that, I realise) as you did the
initial triage.

Sorry about that ...

Right, here is a first attempt at fixing [1]. Please check in that my intent, as described in the above summary, is the desired behaviour.

In D113086#3183917, @ekieri wrote:

I'll try to find time for another look within the next few days. Please don't forget to answer @clementval 's inline comment, and mark mine as done if you fixed it. And I would prefer if you add a TODO comment in the file header about tests for 'result_image', 'stat', and 'errmsg'.

I'll try to find time for another look within the next few days. Please don't forget to answer @clementval 's inline comment, and mark mine as done if you fixed it. And I would prefer if you add a TODO comment in the file header about tests for 'result_image', 'stat', and 'errmsg'.

Thanks for addressing my comments! I noted an oversight inline, and wonder about the intended test coverage, otherwise this looks good I think.

In D113086#3162638, @ktras wrote:

In D113086#3145204, @ekieri wrote:

It would be nice with a comment citing where in the standard the interface of this intrinsic is specified.

Are you (or do you know of someone who is) planning to write the actual semantics checks as well in the near future? I am a little concerned that otherwise the developer who does that one day will be unaware of these tests, and implements additional tests of their own, leaving your tests with XFAIL.

Also, your test contains trailing whitespace. Is this intentional? The coding standards [1] discourages that, and while they primarily concern the c++ code, the argument provided is no less valid for test cases in fortran. On the other hand, @klausler made the (also valid) argument in D113077 that variations in style improves test coverage of the parser. I am fine with you doing it either way, but encourage you to make a conscious decision about it.

[1] https://llvm.org/docs/CodingStandards.html#whitespace

@ekieri, in response to one of your questions, I am currently working on adding the collectives to flang/lib/Evaluate/intrinsics.cpp. When I submit patches, they will include changes to the corresponding collective test to remove the XFAIL directive. However, regardless of who works on the collectives, when a test with an XFAIL directive does not fail, the test will be listed as Unexpectedly Passed when running the test suite. This should bring any developer's attention to these tests that @rouson is submitting.

It would be nice with a comment citing where in the standard the interface of this intrinsic is specified.

Can you please base the patch on main? This diff looks like a small typo fix, but the whole file is new. That would also help the pre-merge tester to apply your patch.

In D113086#3139018, @rouson wrote:

@kiranchandramohan with your approval and that of @ktras, I just pushed differential D113076. Could you review and approve this one along with differential D113077, D113083, and D113084? Also, could you point me to a policy on what approvals are needed in order to push commits to main?

In D113706#3125805, @klausler wrote:

Standard conformance notwithstanding, a procedure pointer should not be allowed to be associated with any target procedure (or pointer) that is not callable via the pointer after the association.

Thanks for your efforts! Looks good to me, but please wait for Peter. I left a few suggestions inline that you could consider.

I agree with your interpretation of paragraph 3 (and Peter's of paragraph 4). However, both gfortran and nvfortran allow pointers with explicit interface to target procedures with implicit interface, if my test case is correct. Perhaps it could be useful to test with other compilers? And _if_ most of them take the more liberal standpoint on this, do we keep to the standard, or is it more user-friendly to implement an established extension? Or could the standard be read differently? No strong opinion from my side (but my instinct is to stay with the standard), just wanted to bring this to your attention.

That worked out nicely, thanks a lot for all your help!

In D112381#3090946, @PeteSteinfeld wrote:

In D112381#3085643, @ekieri wrote:

Am I not supposed to havet a track record within the project to get commit access? The policy you linked suggests that. This is my first contribution to llvm.

@ekieri, I've discussed this with Peter, and we both think that the best thing is for you to get commit access and do the commit yourself.

Am I not supposed to havet a track record within the project to get commit access? The policy you linked suggests that. This is my first contribution to llvm.

Thanks @klausler for your comments!

Address comments from Peter

Thanks @PeteSteinfeld for the review! I also added a comment for C1519 (initialisation of procedure pointers). All aspects of those constraints are not tested, but I intend to continue working towards that in subsequent patches.

Address comment from Pete