[flang-commits] [PATCH] D113086: [flang] Add a semantics test for co_reduce

[Emil Kieri via Phabricator via flang-commits]

ekieri added a comment.

In D113086#3162638 <https://reviews.llvm.org/D113086#3162638>, @ktras wrote:

> In D113086#3145204 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

Great, thanks, as you are working on that I see no issue. I also thought that 'Unexpectedly Passed' was unlikely to trigger due to differences in the error messages, but as all messages are pre-existing (as you kindly pointed out) I admit this risk is small. But still, good that you are working on the checks!



================
Comment at: flang/test/Semantics/collectives05.f90:41
+    ! the error is seen as too many arguments to the co_reduce() call
+    !ERROR: too many actual arguments for intrinsic 'co_reduce'
+    call co_reduce(i, int_op, result_image=1, stat=status, errmsg=message, 3.4)
----------------
ktras wrote:
> ekieri wrote:
> > Please start error messages with a captial letter, as per [1]. (I suppose that overrides [2], which requires the opposite.) Same things on line 45. 
> > 
> > [1] https://flang.llvm.org/docs/C++style.html#error-messages
> > [2] https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
> @rouson is not creating error messages here, he is matching the error message that will be produced by the compiler once collectives have been added to the list of intrinsics. If the error message not beginning with a capital letter is an issue, the source code that currently procedures this message would need to be edited. I personally would think that that might be outside the scope of this patch, as it is just adding a test, not editing source code.
Indeed, sorry, and thanks for pointing that out. I agree completely, fixing that kind of formatting is another patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113086/new/

https://reviews.llvm.org/D113086