test for standard conformance #205

MarDiehl · 2020-05-30T17:14:03Z

There are several compiler options that help to detect non-standard conformant code.
I have included some of them (only for Intel and GNU compiler).
Unfortunately, the result is that the code does not compile:

Intel compiler:
/tmp/stdlib/build/src/stdlib_experimental_linalg_diag.f90(7): error #6645: The name of the module procedure conflicts with a name in the encompassing scoping unit.   [DIAG_RSP]
      function diag_rsp(v) result(res)
GNU compiler
/tmp/stdlib/src/tests/optval/test_optval.f90:252:15:

  252 |     z = optval(x, [2.0_qp, -2.0_qp])
        |               1
        Error: ‘x’ at (1) is an array and OPTIONAL; IF IT IS MISSING, it
        cannot be the actual argument of an ELEMENTAL procedure unless
        there is a non-optional argument with the same rank (12.4.1.5)
        [-Werror=pedantic]

There is also a high number of warnings related to type conversions.

also one quick fix: complex => cmplx note that the current code does not compile Intel compiler: /tmp/stdlib/build/src/stdlib_experimental_linalg_diag.f90(7): error #6645: The name of the module procedure conflicts with a name in the encompassing scoping unit. [DIAG_RSP] function diag_rsp(v) result(res) GNU compiler /tmp/stdlib/src/tests/optval/test_optval.f90:252:15: 252 | z = optval(x, [2.0_qp, -2.0_qp]) | 1 Error: ‘x’ at (1) is an array and OPTIONAL; IF IT IS MISSING, it cannot be the actual argument of an ELEMENTAL procedure unless there is a non-optional argument with the same rank (12.4.1.5) [-Werror=pedantic]

milancurcic · 2020-05-30T17:53:11Z

Martin, thank you and welcome! I think this will be a great step forward. I suggest that we work our way through the errors raised in this PR and fix them until all tests pass.

certik · 2020-05-30T18:05:41Z

In Fortran, are you supposed to use cmplx or complex?

milancurcic · 2020-05-30T18:15:51Z

cmplx is an intrinsic conversion function (like int, real, and char) and complex is the name of the type. I confused this myself many times before but the compiler always corrected me (I don't remember which one). In this case, it's a GNU extension.

certik · 2020-05-30T18:25:13Z

Another pitfall the compiler should provide a nice error / warning message. Thanks!

jvdp1 · 2020-05-30T18:43:54Z

Thank you for starting this.
I am not a huge fan of -Wconversion because it reports many implicit conversions. Others will help us to avoif pitfalls like complex vs cmplx.

MarDiehl · 2020-05-30T18:56:50Z

Martin, thank you and welcome! I think this will be a great step forward. I suggest that we work our way through the errors raised in this PR and fix them until all tests pass.

For gfortran, the optval tests fail:

Error: ‘x’ at (1) is an array and OPTIONAL; IF IT IS MISSING, it cannot be the actual argument of an ELEMENTAL procedure unless there is a non-optional argument with the same rank (12.4.1.5)

I'm also not sure if you are even allowed to pass in an optional argument into a function. Is x in optval present or not?

MarDiehl · 2020-05-30T18:58:17Z

Thank you for starting this.
I am not a huge fan of -Wconversion because it reports many implicit conversions. Others will help us to avoif pitfalls like complex vs cmplx.

I would say for a statically typed language, these warnings are reasonable. Once just has to carefully set the types (5 is not 5.)

milancurcic · 2020-05-30T19:43:23Z

I'm also not sure if you are even allowed to pass in an optional argument into a function. Is x in optval present or not?

You can pass it to a procedure if the corresponding dummy argument is also declared optional.

It looks like we'll need to remove the elemental attribute from optval and use fypp to create specific procedures for all ranks.

jvdp1 · 2020-05-30T19:46:27Z

It looks like we'll need to remove the elemental attribute from optval and use fypp to create specific procedures for all ranks.

I am currently trying this option. However, we then must check is the actual arguments have the same dimension, and use stop error if it is not the case. This was ensure by the elementatal function. The pure statement might need to be removed if a message is provided.

MarDiehl · 2020-05-30T20:23:37Z

It looks like we'll need to remove the elemental attribute from optval and use fypp to create specific procedures for all ranks.

I am currently trying this option. However, we then must check is the actual arguments have the same dimension, and use stop error if it is not the case. This was ensure by the elementatal function. The pure statement might need to be removed if a message is provided.

To be honest, I have problems to understand the error of gfortran. What does there is means? Does it refer to the calling functions or to the called (elemental) function? optval has a second, non-optional argument.

jvdp1 · 2020-05-30T20:46:05Z

It looks like we'll need to remove the elemental attribute from optval and use fypp to create specific procedures for all ranks.

I am currently trying this option. However, we then must check is the actual arguments have the same dimension, and use stop error if it is not the case. This was ensure by the elementatal function. The pure statement might need to be removed if a message is provided.

To be honest, I have problems to understand the error of gfortran. What does there is means? Does it refer to the calling functions or to the called (elemental) function? optval has a second, non-optional argument.

My first impression was that the message was clear (to me at least). To be sure, I went to the gcc source code.
Now with this comment in this code:

/* If it is an array, it shall not be supplied as an actual argument
     to an elemental procedure unless an array of the same rank is supplied
     as an actual argument corresponding to a nonoptional dummy argument of
     that elemental procedure(12.4.1.5).  */

, I am doubting.

MarDiehl · 2020-05-30T21:25:42Z

It looks like we'll need to remove the elemental attribute from optval and use fypp to create specific procedures for all ranks.

I am currently trying this option. However, we then must check is the actual arguments have the same dimension, and use stop error if it is not the case. This was ensure by the elementatal function. The pure statement might need to be removed if a message is provided.

To be honest, I have problems to understand the error of gfortran. What does there is means? Does it refer to the calling functions or to the called (elemental) function? optval has a second, non-optional argument.

My first impression was that the message was clear (to me at least). To be sure, I went to the gcc source code.
Now with this comment in this code:
/* If it is an array, it shall not be supplied as an actual argument
     to an elemental procedure unless an array of the same rank is supplied
     as an actual argument corresponding to a nonoptional dummy argument of
     that elemental procedure(12.4.1.5).  */
, I am doubting.

Same here. In case that an optional argument is not given, there is still the other argument to determine the properties of the return value.
We should discuss report it to the gfortran developers. I will also check if it compiles with Intel.

jvdp1 · 2020-05-30T21:28:58Z

From the Fortran Standard (15.5.2.12):

3 An optional dummy argument that is not present is subject to the following restrictions.
.....
(6) If it is an array, it shall not be supplied as an actual argument to an elemental procedure unless an
array of the same rank is supplied as an actual argument corresponding to a nonoptional dummy
argument of that elemental procedure.
....

I understand the same from this sentence.

We should discuss report it to the gfortran developers. I will also check if it compiles with Intel.

I agree. Btv, which version of gfortran did you use (I used GFortran9.3)?

MarDiehl · 2020-05-30T21:30:32Z

I agree. Btv, which version of gfortran did you use (I used GFortran9.3)?

10.1

jvdp1 · 2020-05-30T21:49:47Z

Intel compiler:
/tmp/stdlib/build/src/stdlib_experimental_linalg_diag.f90(7): error #6645: The name of the module procedure conflicts with a name in the encompassing scoping unit. [DIAG_RSP]
function diag_rsp(v) result(res)

This issue related to the Intel compiler should be solved by #208

MarDiehl · 2020-05-30T22:47:12Z

Gfortran issue reported as bug on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95446

MarDiehl · 2020-05-31T21:26:46Z

in general, ready to merge.
But please discuss whether you are ok with all the warnings. I usually prefer to turn on all compiler warnings and remove them step by step. A few false positives will always remain.
The most of the warnings here are related to type conversions and can be easily fixed by introducing explicit type conversions.

jvdp1

Thank you.
LGTM. For the false positives, would it be possible/adequate to output a warning for the users?

src/CMakeLists.txt

MarDiehl · 2020-05-31T21:47:48Z

For the false positives, would it be possible/adequate to output a warning for the users?
Which false positives do you mean?

From my side, it was just a general comment that sometimes compilers give warnings despite the code is valid code. For example there are warnings about uninitialized variables even though the the choice of variables of the calling function makes it impossible to use a variable without initialization.
There are at least three ways to solve this:

Write some boilerplate code that silences the compiler
Turn off the warning selectively (not always possible)
Have two build configurations. A picky one for development (developers need to get used to a certain amount of warnings) and one for production that does not scares normal users.

jvdp1 · 2020-06-01T07:29:24Z

For the false positives, would it be possible/adequate to output a warning for the users?
Which false positives do you mean?

From my side, it was just a general comment that sometimes compilers give warnings despite the code is valid code. For example there are warnings about uninitialized variables even though the the choice of variables of the calling function makes it impossible to use a variable without initialization.

# prevent false positive (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95446)
if(CMAKE_Fortran_COMPILER_ID STREQUAL GNU)
  set_source_files_properties("test_optval.f90" PROPERTIES COMPILE_FLAGS "-Wno-error=pedantic")
endif()

I meant such false positives, that are due to, e.g. a bug in the compiler. If someone introduces new procedures in this test, he/she might want to know that these warnings are removed. However, if there is no warnings, he will need to check the CMakefile to know it.

There are at least three ways to solve this:

Write some boilerplate code that silences the compiler

Turn off the warning selectively (not always possible)

Have two build configurations. A picky one for development (developers need to get used to a certain amount of warnings) and one for production that does not scares normal users.

IMO we should go for the 3rd option. The 1st option should never be taken, and the 2nd option should be avoided as much as possible.

thanks for the feedback

jvdp1

Thank you for these changes. +1 to merge

milancurcic

Looks good! Thank you, Martin.

certik

The changes seem fine to me. Thanks!

certik · 2020-06-04T16:03:03Z

3 positive reviews, merging.

MarDiehl added 3 commits May 30, 2020 19:09

option was only introduced in gfortran 10.0

d2dc3ec

support for older gfortran

285e3a1

jvdp1 mentioned this pull request May 30, 2020

proposition for solving an issue with optval #206

Closed

jvdp1 mentioned this pull request May 30, 2020

Fix for an issue in stdlib_experimental_linalg_diag with Intel compiler #208

Merged

MarDiehl added 5 commits May 31, 2020 19:10

Merge branch 'master' into standard-checks

a7becc3

language

a206e25

allow non-standard conforming code only selectively where needed

022fcf7

prevent erroneous rejection of valid code

999be0b

order was mixed up, no global relaxation of standards

4378a67

jvdp1 reviewed May 31, 2020

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

MarDiehl added 2 commits June 1, 2020 09:41

polishing

f563893

thanks for the feedback

following standard, no exception for gfortran 10 needed

0abec4b

jvdp1 approved these changes Jun 1, 2020

View reviewed changes

jvdp1 requested review from certik and milancurcic June 1, 2020 10:00

milancurcic mentioned this pull request Jun 4, 2020

fix build failure with nagfor7.0 because of using "complex" function #210

Merged

milancurcic approved these changes Jun 4, 2020

View reviewed changes

certik approved these changes Jun 4, 2020

View reviewed changes

certik merged commit 48b28e8 into fortran-lang:master Jun 4, 2020

awvwgk mentioned this pull request Jan 18, 2021

Intel Fortran and -standard-semantics #298

Closed

test for standard conformance #205

test for standard conformance #205

Uh oh!

Conversation

MarDiehl commented May 30, 2020

Uh oh!

milancurcic commented May 30, 2020

Uh oh!

certik commented May 30, 2020

Uh oh!

milancurcic commented May 30, 2020

Uh oh!

certik commented May 30, 2020

Uh oh!

jvdp1 commented May 30, 2020

Uh oh!

MarDiehl commented May 30, 2020

Uh oh!

MarDiehl commented May 30, 2020

Uh oh!

milancurcic commented May 30, 2020

Uh oh!

jvdp1 commented May 30, 2020

Uh oh!

MarDiehl commented May 30, 2020

Uh oh!

jvdp1 commented May 30, 2020

Uh oh!

MarDiehl commented May 30, 2020

Uh oh!

jvdp1 commented May 30, 2020

Uh oh!

MarDiehl commented May 30, 2020

Uh oh!

jvdp1 commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarDiehl commented May 30, 2020

Uh oh!

MarDiehl commented May 31, 2020

Uh oh!

jvdp1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarDiehl commented May 31, 2020

Uh oh!

jvdp1 commented Jun 1, 2020

Uh oh!

jvdp1 left a comment

Choose a reason for hiding this comment

Uh oh!

milancurcic left a comment

Choose a reason for hiding this comment

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

certik commented Jun 4, 2020

Uh oh!

Uh oh!

jvdp1 commented May 30, 2020 •

edited

Loading