- User Since
- Oct 14 2021, 1:15 PM (42 w, 3 d)
Jun 15 2022
Jun 14 2022
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.
Jun 8 2022
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?
Jun 7 2022
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?
May 28 2022
May 26 2022
May 22 2022
May 5 2022
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.
May 3 2022
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!
May 2 2022
May 1 2022
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?
Apr 26 2022
Thanks for addressing my comments!
Apr 23 2022
Thanks for taking the time to work on quality and compliance! LGTM, but please consider my nits.
Apr 13 2022
Apr 12 2022
Apr 11 2022
One nit/question inline, otherwise LGTM.
Apr 7 2022
Apr 6 2022
Ok, good. Then please abandon this revision (Add Action... down by the comment box), so that its status is clear to everybody. Cheers!
Apr 5 2022
Hi! Thanks for this patch, and for your interest in contributing to Flang!
Thank you all for the support!
Apr 4 2022
This patch is the result of discussions in https://reviews.llvm.org/D122542
Apr 1 2022
Mar 28 2022
Address review comment.
Fix typo in summary.
Mar 27 2022
Mar 25 2022
Mar 23 2022
Mar 16 2022
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
Mar 15 2022
Sorry about that ...
Right, here is a first attempt at fixing . Please check in that my intent, as described in the above summary, is the desired behaviour.
Dec 13 2021
Dec 9 2021
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'.
Dec 5 2021
Thanks for addressing my comments! I noted an oversight inline, and wonder about the intended test coverage, otherwise this looks good I think.
Dec 1 2021
Nov 21 2021
It would be nice with a comment citing where in the standard the interface of this intrinsic is specified.
Nov 18 2021
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.
Nov 14 2021
Thanks for your efforts! Looks good to me, but please wait for Peter. I left a few suggestions inline that you could consider.
Nov 11 2021
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.
Nov 9 2021
Nov 7 2021
Oct 28 2021
That worked out nicely, thanks a lot for all your help!
Oct 27 2021
Oct 25 2021
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