Fortran-FOSS-Programmers / Best_Practices

the opinionated *best practices* of Fortran FOSS Programmers group
Creative Commons Attribution Share Alike 4.0 International
65 stars 10 forks source link

Is *explicit* better than implicit? #10

Open szaghi opened 8 years ago

szaghi commented 8 years ago

I feel that there is an almost unanimous agreement on that implicit none must be mandatory, but I guess many of you will provide interesting exceptions, so

do you agree to advice to be explicit?

In the case you disagree, please elaborate your idea.

cmacmackin commented 8 years ago

I can think of no circumstance in which explicit none should be omitted. Put it at the start of every module, submodule, or program.

On 16-01-18 04:35 PM, Stefano Zaghi wrote:

I feel that there is an almost unanimous agreement on that |implicit none| must be mandatory, but I guess many of you will provide interesting /exceptions/, so

do you agree to advice to be explicit?

In the case you disagree, please elaborate your idea.

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10.

Chris MacMackin cmacmackin.github.io http://cmacmackin.github.io

LadaF commented 8 years ago

I think implicit none can be omitted for procedures which get implicit none from their host, both internal and module procedures. At least that is my personal style and I omit it in this case always.

2016-01-18 16:36 GMT+00:00 Chris MacMackin notifications@github.com:

I can think of no circumstance in which explicit none should be omitted. Put it at the start of every module, submodule, or program.

On 16-01-18 04:35 PM, Stefano Zaghi wrote:

I feel that there is an almost unanimous agreement on that |implicit none| must be mandatory, but I guess many of you will provide interesting /exceptions/, so

do you agree to advice to be explicit?

In the case you disagree, please elaborate your idea.

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10.

Chris MacMackin cmacmackin.github.io http://cmacmackin.github.io

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-172579622 .

cmacmackin commented 8 years ago

Agreed. So long as they are within a scope in which implicit none has been specified. (Hence why I didn't list functions and subroutines as places where it should be specified.)

On 16-01-18 04:41 PM, LadaF wrote:

I think implicit none can be omitted for procedures which get implicit none from their host, both internal and module procedures. At least that is my personal style and I omit it in this case always.

2016-01-18 16:36 GMT+00:00 Chris MacMackin notifications@github.com:

I can think of no circumstance in which explicit none should be omitted. Put it at the start of every module, submodule, or program.

On 16-01-18 04:35 PM, Stefano Zaghi wrote:

I feel that there is an almost unanimous agreement on that |implicit none| must be mandatory, but I guess many of you will provide interesting /exceptions/, so

do you agree to advice to be explicit?

In the case you disagree, please elaborate your idea.

— Reply to this email directly or view it on GitHub

https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10.

Chris MacMackin cmacmackin.github.io http://cmacmackin.github.io

— Reply to this email directly or view it on GitHub

https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-172579622 .

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-172581895.

Chris MacMackin cmacmackin.github.io http://cmacmackin.github.io

szaghi commented 8 years ago

@LadaF sure, I agree: in those cases it is unnecessary, but my question was, indeed,

have some of you good reasons to use implicit defined variables (as the older of us learned to do with F66/77)?

LadaF commented 8 years ago

There are some tricks for poor man's templates where instead of C macros one can use implicit typing and standard include statement.

consider

#define mytype type(abc)
subroutine abc_sub
#include "sub_base-inc-1.f90"
end subroutine
#undef mytype

with "sub_base-inc-1.f90" containing

mytype :: var

versus

subroutine abc_sub
    implicit type(abc) (v)
    include "sub_base-inc-2.f90"
end subroutine

with "sub_base-inc.f90" containing just the usage of variable var. I am not sure if I got the implicit statement completely right, as I never use it.

nncarlson commented 8 years ago

Very interesting trick @LadaF. Not sure I'd ever use it, but it's interesting nonetheless.

I'd go a step beyond making implicit none mandatory at the start of any module or program. I'd also require that it not appear at the start of any module or internal procedure which get it from their host. Programmers learn by example and the takeaway message by having it there is that it must be necessary somehow, and mislead an unaware programmer.

szaghi commented 8 years ago

@LadaF thank you very much, I win my bet, I guessed that I can learn something even by implicit! I love to learn something new that I never read, your trick is very appreciated!

@nncarlson I am sorry, but I completely miss your point. Can you elaborate more on not appear at the start of...?

nncarlson commented 8 years ago

@szaghi, If implicit none is present at the start of a module (and I think it should be required) then it applies to the entire module scope and it is not necessary to put it also at the start of the contained procedures, as others have noted. One can go ahead anyway and add implicit none at the start of contained procedures, but it is redundant and unnecessary, and conveys the false idea that it is required. For those reasons I'd require that implicit none not be declared there, but only at the module/program scope. As I said, programmers learn by example.

cmacmackin commented 8 years ago

I second requiring it not to be declared at the start of internal/module procedures.

On 16-01-18 09:50 PM, Neil Carlson wrote:

@szaghi https://github.com/szaghi, If implicit none is present at the start of a module (and I think it should be required) then it applies to the entire module scope and it is not necessary to put it also at the start of the contained procedures, as others have noted. One can go ahead anyway and add implicit none at the start of contained procedures, but it is redundant and unnecessary, and conveys the false idea that it is required. For those reasons I'd require that implicit none not be declared there, but only at the module/program scope. As I said, programmers learn by example.

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-172663312.

Chris MacMackin cmacmackin.github.io http://cmacmackin.github.io

raullaasner commented 8 years ago

This could be part of a general rule: do not write code that has no effect. The implicit none scope is an example of where such a rule is not self-evident, for beginners at least.

szaghi commented 8 years ago

@nncarlson and all, sorry, this was so obvious for me that I was confused before reach the second, crucial, argument that @raullaasner clarifies

do not write code that has no effect

Wonderful, great zen! Thank you very much.

nncarlson commented 8 years ago

@raullaasner is spot on with the general rule. Another example that comes to mind (sorry drifting off-topic) is deallocation of local allocatable variables. Don't bother with explicitly deallocating them before returning to the caller -- it happens automatically. Explicitly deallocating them conveys the false notion to the naive that it is necessary to avoid memory leaks.

raullaasner commented 8 years ago

@nncarlson If you want to check the deallocation status and catch any error messages, you would need to do that manually.

nncarlson commented 8 years ago

@raullaasner, that's true of course. I'm curious though who actually checks the deallocation status, and have they ever seen a deallocation failure. But don't answer! -- a topic for a different thread when the time is ripe (make a note @szaghi :-)

Tobychev commented 8 years ago

The first time I used fortran seriously, I had to introduce a calculation into a larger legacy simulation, and I made some typos. It took me close to a month to figure out why the program was compiling and then returning nonsense.

What is the cost of excessive "implicit none" that can be compared with the cost of leaving it out once in error?

szaghi commented 8 years ago

@Tobychev In the past I compulsively added implicit none everywhere just because others had the possibility to take my snippets and use them into old-fashioned files-library where, without module/program encapsulation and/or explicit interfaces, all the procedures live in a blob-ed namespace without interfaces. In this scenario, I was happy of this bad approach just because save my time in eventual subsequent debugging of others codes.

However, when we talk about best practices, I think that the @raullaasner and @nncarlson advice is very important: do not write code that has no effect. If we agree to suggest that implicit none must be added at the start of each module, submodule, program, it should be also good to advice (the eventually new Fortraners) that implit none is not necessary also inside procedures, because it is inherited from host. Obviously, if procedures have no host, put implicit none inside them, but we should not promote old-fashioned files-library, ops... this must go into another discussion, I am taking note @nncarlson :-)

The our point is avoid to promote false statements, i.e. implicit none is needed everywhere: as @nncarlson highlights examples are of paramount relevance for strange people like us.

I agree with you that excessive implicit none has not so high cost, but it is simply not necessary (except for particular scenario, e.g. old-legacy codes with a lot of implicit definitions).

See you soon.

Tobychev commented 8 years ago

@szaghi While I agree that avoiding writing code that has no effect is a good goal, it is clearly much more desirable to write code that gives correct results, and efforts to be correct trumps efforts to avoid dead code.

For example: mandating that a select statement always have a default clause is a sensible thing even though it might never be reached because the code always chooses one of the earlier branches. Should we then, following the no-dead-code principle, mandate that you should never use a default statement if you are really sure the code will take one of the earlier branches? I would say this is excessive and dangerous advice considering the cost of retaining it (a bit of code that can be optimised away by the compiler) versus the cost of programmer misjudgement ( unexpected behaviour in rare cases ).

It is also possible to argue that implicit none should never be included by the strict no-dead-code principle since the same effect can be achieved by use of compiler flags, presuming this option exists in most relevant compilers. I for one always use the "implicit none" compiler flag, but include the statements in program code anyway. I've read arguments that this is good practice because people could copy and build your code without your flags, but honestly its mostly as a tribute to that month I spend trying to get my stupid routine to make sense...

Anyway, practice my objection is aimed at the prohibition of implicit none in certain places: I feel this is excessive considering the great cost that can follow from mistakenly leaving it out, and the small cost of its excessive use. I think the style guide should only speak of where it must appear, because there is simply no strong argument against including it excessively.

I also think this thread has now derailed from the issue of where implicit can be used, and is now a debate about the exact style of how it should be forbidden. (Sorry that I helped with that).

szaghi commented 8 years ago

@Tobychev great! All interesting arguments!

it is clearly much more desirable to write code that gives correct results

I agree.

mandating that a select statement always have a default clause is a sensible thing...

Wonderful! I am taking note, thank you very much!

It is also possible to argue that implicit none should never be included by the strict no-dead-code principle since the same effect can be achieved by use of compiler flags, presuming this option exists in most relevant compilers.

I partially disagree: I do no like to rely on compiler vendors when a standard syntax is available. I vote for a guideline advising to use implicit none statement and eventually to use the compiler option if available, but not the contrary.

practice my objection is aimed at the prohibition of implicit none in certain places: I feel this is excessive considering the great cost that can follow from mistakenly leaving it out, and the small cost of its excessive use. I think the style guide should only speak of where it must appear, because there is simply no strong argument against including it excessively.

I agree, but I think that we should not talk about prohibition neither about costs: @nncarlson and others suggestions should be viewed (IMO) as an advice that

if host has implicit none, the contained procedures do no need it or as

convey right Fortran features with correct examples

We are concerning that if we advice to insert implicit none everywhere without consciousnesses we could mislead inexperienced Fortran on the wrong conviction that it is necessary. We could agree that implicit none everywhere could be helpful to save us from subtle bugs (in particular circumstances), but we could not agree that it is necessary.

Do not worry to drive me far from this topic, I am learning a lot from such a digression, thank you very much!

Returning on topic, what do you think about something like the guideline I wrote in my guidelines? Is it comprehensive enough to catch the above suggestions? At least, the compiler-option comment is missing, I guess.

See you soon.

zbeekman commented 8 years ago

Should we then, following the no-dead-code principle, mandate that you should never use a default statement if you are really sure the code will take one of the earlier branches?

I may be in the minority here, or this opinion may be considered "extreme" by some, but I would actually argue yes to this point. Increasing the size of a codebase and the inclusion of unreachable code is a no-no in my book. Yes it may be seen as defensive coding, but if it's actually impossible to enter that branch of the select statement (even with potentially incorrect user input, corrupted data files, or out of memory errors) then it is "dead" code, and exists for no other reason than to give you a warm fuzzy feeling that you have been a diligent defensive programmer. Additionally this also means there is no way to test its correctness.

My philosophy is that you should strive to get 100% test coverage during unit tests, regression tests and integration tests. If you can't devise a way to execute a branch of an if statement, or a case from a select case construct during testing, because it is impossible to reach, even with bad user input etc. then that code should be removed. There is no point having it, and it distracts and bloats code from the important parts. Further more, in my experience, assuming a constant error rate (N_errors/statement) has been a decent assumption. As a corollary, the longer you make your code, the more bugs you will introduce. Of course this is a gross approximation, but once you start adding a lot of "dead" code, there's no way to test if it is actually correct, and it obfuscates the other parts of the code that are executed by making them further apart on the page, etc.

This ties in with my software architecture/implementation guiding principal:

"Don't build it until you need it."

I used to spend a lot of time, and effort thinking about ways that I might want to use a given code in the future, and starting to implement bits and pieces of that to make it "generic" and "future proof." I wasted a lot of time and introduced a lot of bugs. It is much better, IMO to start with a solid object oriented architecture, that is designed to be extensible and generic through use of abstract classes, TBPs etc. but then only build out the concrete implementations that you NEED RIGHT NOW.

It is also possible to argue that implicit none should never be included by the strict no-dead-code principle since the same effect can be achieved by use of compiler flags, presuming this option exists in most relevant compilers. I for one always use the "implicit none" compiler flag, but include the statements in program code anyway. I've read arguments that this is good practice because people could copy and build your code without your flags, but honestly its mostly as a tribute to that month I spend trying to get my stupid routine to make sense...

I would argue that compiler flags are not standard and therefore, passing the compiler flag as an alternative to implicit none does not imply that including implicit none is writing code that has no effect.

Anyway, practice my objection is aimed at the prohibition of implicit none in certain places: I feel this is excessive considering the great cost that can follow from mistakenly leaving it out, and the small cost of its excessive use. I think the style guide should only speak of where it must appear, because there is simply no strong argument against including it excessively.

I think the arguments against including it excessively have been enumerated already: Teaching by example that n00bs need to add it everywhere, increasing the code length, and possibly including it where it is not needed, but forgetting it where it is needed.

However, I concede that treating this too rigorously is probably excessive. I think it is very important to say something like "you must use implicit none at the beginning of every module and program declaration" and then include the motto "Don't write dead, unreachable code or code that has no effect" but explicitly banning implicit none in other contexts seems a bit heavy handed to me.

This also raises a final question in my mind: submodules

I don't know enough about them yet, and have never tried using them, but it is conceivable that numerous submodules may have the ability to be reused and integrated into different parent modules. If this is the case, I think you should be declaring implicit none in the submodule if it is allowed to be compiled separately from the main module, or attached to different modules. I don't know if there is a mechanism for adding implicit none at the top of a submodule, or if it inherits it from the parent module. If there's no means of declaring it locally at the top of the file, then I think it will have to be declared in each of the procedures, to explicitly communicate that implicit typing is prohibited.

FWIW (pobably not much! :smile: ) these views are solely my personal views and preferences and I respect everyones views on this forum. I just wanted to share my logic, and stimulate further conversation.

szaghi commented 8 years ago

these views are solely my personal views and preferences and I respect everyones views on this forum. I just wanted to share my logic, and stimulate further conversation.

This should be valid for all of us :smile:

This is a collaborative effort trying to summarize our views (not only mine, your, of @specific_member).

@zbeekman I agree on all.

cmacmackin commented 8 years ago

@zbeekman I'm pretty sure that you can declare implicit none at the top of a submodule--a piece of example code someone had written as a demonstration (see below), and which I use to test FORD, had it at any rate. I had assumed that this wouldn't be inherited from the parent, but I don't think I ever actually read that anywhere.

!! Demonstration of Fortran 2008 submodules
! J. Overbey - 1 Dec 2009

!! The module and submodules have the following hierarchy:
!!
!!               module
!!                  |
!!                  |
!!             submodule1
!!                  |
!!                  |
!!             submodule2
!!                 /|\
!!               /  |  \
!!             /    |    \
!!           /      |      \
!! submodule3  submodule4  submodule5

module module
  implicit none
contains ! Empty contains section allowed in Fortran 2008
end module module

submodule (module) submodule1
  implicit none
end

submodule (module : submodule1) submodule2
end submodule

submodule (module : submodule2) submodule3
!! Documentation!
end submodule submodule3

submodule (module : submodule2) submodule4
endsubmodule

submodule (module : submodule2) submodule5
endsubmodule submodule3
zbeekman commented 8 years ago

Another case, worth considering, is if one writes generic procedures, and then Fortran includes them into a module. In that case I would argue that implicit none is required for the procedure, since there is no guarantee or context when viewing the stand alone procedure, that implicit none will ever be applied.

cmacmackin commented 8 years ago

:+1:, although personally I have a strong dislike of include statements and have never been satisfied with them as a technique for generic programming.

rouson commented 8 years ago

On Jan 20, 2016, at 6:07 AM, Izaak Beekman notifications@github.com wrote:

Should we then, following the no-dead-code principle, mandate that you should never use a default statement if you are really sure the code will take one of the earlier branches?

I may be in the minority here, or this opinion may be considered "extreme" by some, but I would actually argue yes to this point. Increasing the size of a codebase and the inclusion of unreachable code is a no-no in my book. Yes it may be seen as defensive coding, but if it's actually impossible to enter that branch of the select statement (even with potentially incorrect user input, corrupted data files, or out of memory errors) then it is "dead" code, and exists for no other reason than to give you a warm fuzzy feeling that you have been a diligent defensive programmer. Additionally this also means there is no way to test its correctness.

I haven’t read much of this dialogue yet, but I’ll briefly add that dead code could lead to confusion

subroutine cognitive_dissonance(only_2_possibilities) logical, intent(in) :: only_2_possibilities select case(only_2_possibilities) case(.true.) print ,"Sure." case(.false.) print ,"No worries." case default print *,”Hmm... please remind me why I wrote this. There must have been a reason." end select end subroutine

My philosophy is that you should strive to get 100% test coverage during unit tests, regression tests and integration tests.

Excellent point. If you can't devise a way to execute a branch of an if statement, or a case from a select case construct during testing, because it is impossible to reach, even with bad user input etc. then that code should be removed. There is no point having it, and it distracts and bloats code from the important parts. Further more, in my experience, assuming a constant error rate (N_errors/statement) has been a decent assumption.

There is empirical data to back this up. I cite it in the 14th slide of this presentation:

https://www.image.ucar.edu/public/TOY/2008/focus2/Presentations/TALKRouson.pdf

(sorry for not numbering the slides)

As a corollary, the longer you make your code, the more bugs you will introduce. Of course this is a gross approximation, but once you start adding a lot of "dead" code, there's no way to test if it is actually correct, and it obfuscates the other parts of the code that are executed by making them further apart on the page, etc.

Dead code also violates the agile development method of test-driven development (TDD) in which one writes only enough code to pass the test. The test is the specification for what must be written. If one wants more code to be written, then one must say so in a test.

Damian

tclune commented 8 years ago

Of course, if there are really are only 2 possibilities, then the entire section would be clearer with a logical. And for an integer, do you really want the reader to form a proof that the ‘default’ block cannot be reached. I’d recommend “always” having a default block, but putting it at a low priority. :-) Best practices can be a real time keeper, so prioritize for the cases that are more likely to cause problems.

On a slightly related note, what do others think about explicitly including the procedure name in the end clause. E.g.,

subroutine foo( …) end SUBROUTINE FOO

One could omit “FOO” or “SUBROUTINE FOO” and have the same code.

In the good old days, I liked the fact that my editor did the autocompletion for me, and for long procedures, I still think the redundancy is a good thing. (TM) However, for short procedures it merely induces a burden to change 2 lines rather than 1 if I decide to rename the procedure. This is actually a relatively common mistake that I encounter when doing simple refactoring.

So, I’d stop doing it, but have not been bothered enough to go about determining how to disable this aspect of Emacs - esp. in a manner that allows the redundancy for the occasional long procedure. (I still spend a lot of time editing other people’s legacy code with longer procedures.)

Cheers,

On Jan 20, 2016, at 9:37 AM, Damian Rouson notifications@github.com wrote:

On Jan 20, 2016, at 6:07 AM, Izaak Beekman notifications@github.com wrote:

Should we then, following the no-dead-code principle, mandate that you should never use a default statement if you are really sure the code will take one of the earlier branches?

I may be in the minority here, or this opinion may be considered "extreme" by some, but I would actually argue yes to this point. Increasing the size of a codebase and the inclusion of unreachable code is a no-no in my book. Yes it may be seen as defensive coding, but if it's actually impossible to enter that branch of the select statement (even with potentially incorrect user input, corrupted data files, or out of memory errors) then it is "dead" code, and exists for no other reason than to give you a warm fuzzy feeling that you have been a diligent defensive programmer. Additionally this also means there is no way to test its correctness.

I haven’t read much of this dialogue yet, but I’ll briefly add that dead code could lead to confusion

subroutine cognitive_dissonance(only_2_possibilities) logical, intent(in) :: only_2_possibilities select case(only_2_possibilities) case(.true.) print ,"Sure." case(.false.) print ,"No worries." case default print *,”Hmm... please remind me why I wrote this. There must have been a reason." end select end subroutine

My philosophy is that you should strive to get 100% test coverage during unit tests, regression tests and integration tests.

Excellent point. If you can't devise a way to execute a branch of an if statement, or a case from a select case construct during testing, because it is impossible to reach, even with bad user input etc. then that code should be removed. There is no point having it, and it distracts and bloats code from the important parts. Further more, in my experience, assuming a constant error rate (N_errors/statement) has been a decent assumption.

There is empirical data to back this up. I cite it in the 14th slide of this presentation https://www.image.ucar.edu/public/TOY/2008/focus2/Presentations/TALKRouson.pdf (sorry for not numbering the slides). As a corollary, the longer you make your code, the more bugs you will introduce. Of course this is a gross approximation, but once you start adding a lot of "dead" code, there's no way to test if it is actually correct, and it obfuscates the other parts of the code that are executed by making them further apart on the page, etc.

Dead code also violates the agile's test-driven development (TDD) methodology in which one only write just enough code to pass the test. This test is the specification for what must be written. If one wants more code to be written, they must say so in a test.

Damian

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173222701.

Thomas Clune, Ph. D. Thomas.L.Clune@nasa.gov Software Infrastructure Team Lead Global Modeling and Assimilation Office, Code 610.1 NASA GSFC
MS 610.1 B33-C128 Greenbelt, MD 20771 301-286-4635

LadaF commented 8 years ago

My practice is to add the name after the end subroutine if the subroutine gets longer than one page in my editor. It's a subjective criterion and sometimes I forget and add it later when I find end subroutine for which I can't immediately see the beginning with the name.

Vlad

2016-01-20 14:56 GMT+00:00 tclune notifications@github.com:

Of course, if there are really are only 2 possibilities, then the entire section would be clearer with a logical. And for an integer, do you really want the reader to form a proof that the ‘default’ block cannot be reached. I’d recommend “always” having a default block, but putting it at a low priority. :-) Best practices can be a real time keeper, so prioritize for the cases that are more likely to cause problems.

On a slightly related note, what do others think about explicitly including the procedure name in the end clause. E.g.,

subroutine foo( …) end SUBROUTINE FOO

One could omit “FOO” or “SUBROUTINE FOO” and have the same code.

In the good old days, I liked the fact that my editor did the autocompletion for me, and for long procedures, I still think the redundancy is a good thing. (TM) However, for short procedures it merely induces a burden to change 2 lines rather than 1 if I decide to rename the procedure. This is actually a relatively common mistake that I encounter when doing simple refactoring.

So, I’d stop doing it, but have not been bothered enough to go about determining how to disable this aspect of Emacs - esp. in a manner that allows the redundancy for the occasional long procedure. (I still spend a lot of time editing other people’s legacy code with longer procedures.)

Cheers,

  • Tom

On Jan 20, 2016, at 9:37 AM, Damian Rouson notifications@github.com wrote:

On Jan 20, 2016, at 6:07 AM, Izaak Beekman notifications@github.com wrote:

Should we then, following the no-dead-code principle, mandate that you should never use a default statement if you are really sure the code will take one of the earlier branches?

I may be in the minority here, or this opinion may be considered "extreme" by some, but I would actually argue yes to this point. Increasing the size of a codebase and the inclusion of unreachable code is a no-no in my book. Yes it may be seen as defensive coding, but if it's actually impossible to enter that branch of the select statement (even with potentially incorrect user input, corrupted data files, or out of memory errors) then it is "dead" code, and exists for no other reason than to give you a warm fuzzy feeling that you have been a diligent defensive programmer. Additionally this also means there is no way to test its correctness.

I haven’t read much of this dialogue yet, but I’ll briefly add that dead code could lead to confusion

subroutine cognitive_dissonance(only_2_possibilities) logical, intent(in) :: only_2_possibilities select case(only_2_possibilities) case(.true.) print ,"Sure." case(.false.) print ,"No worries." case default print *,”Hmm... please remind me why I wrote this. There must have been a reason." end select end subroutine

My philosophy is that you should strive to get 100% test coverage during unit tests, regression tests and integration tests.

Excellent point. If you can't devise a way to execute a branch of an if statement, or a case from a select case construct during testing, because it is impossible to reach, even with bad user input etc. then that code should be removed. There is no point having it, and it distracts and bloats code from the important parts. Further more, in my experience, assuming a constant error rate (N_errors/statement) has been a decent assumption.

There is empirical data to back this up. I cite it in the 14th slide of this presentation < https://www.image.ucar.edu/public/TOY/2008/focus2/Presentations/TALKRouson.pdf> (sorry for not numbering the slides). As a corollary, the longer you make your code, the more bugs you will introduce. Of course this is a gross approximation, but once you start adding a lot of "dead" code, there's no way to test if it is actually correct, and it obfuscates the other parts of the code that are executed by making them further apart on the page, etc.

Dead code also violates the agile's test-driven development (TDD) methodology in which one only write just enough code to pass the test. This test is the specification for what must be written. If one wants more code to be written, they must say so in a test.

Damian

— Reply to this email directly or view it on GitHub < https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173222701 .

Thomas Clune, Ph. D. Thomas.L.Clune@nasa.gov Software Infrastructure Team Lead Global Modeling and Assimilation Office, Code 610.1 NASA GSFC MS 610.1 B33-C128 Greenbelt, MD 20771 301-286-4635

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173228002 .

nncarlson commented 8 years ago

The default clause issue is an interesting one. I tend to agree with @zbeekman here. My policy is to omit it if it is not needed, with one exception. If the intent is that one of the case stanzas is always executed but it is not completely clear from the immediate context that this will be the case, then I do add the default clause with an assertion:

    case default
      ASSERT(.false.)
    end select

Here ASSERT is part of a simple macro-based DBC system that I use. ASSERT asserts that its argument is true, so the strange idiom ASSERT(.false.) will trigger an error. In this case the default clause serves to document to the reader the intent that one of the case clauses is supposed to be executed.

szaghi commented 8 years ago

To all: wonderful... how many notes I am keeping...

@nncarlson what does DBC mean? It is very interesting, Fortran errors handling is a mistery for me, can you elaborate more?

nncarlson commented 8 years ago

@szaghi, Design-by-contract. The full-blown concept (which I really don't know) is much more involved than my simple use of it with things like preconditions and postconditions. This is built into some languages. C handles it (if I remember correctly) via macros like I am doing. I've got two macros ASSERT and INSIST. ASSERT(expr) expands to a statement that calls an "abort" procedure if expr is not true, which prints out file and line number, and then halts execution. That is unless the code is compiled with the (boolean) macro NDEBUG defined, in which case ASSERT expands to a Fortran comment. INSIST is just like ASSERT except it is always live. The policy is that these should never be triggered; e.g., they are not for checking user input or errors that could result from bad user input -- proper error checking should be used for that.

I've found this to be so incredibly helpful during code development at immediately pinpointing programming errors that would otherwise require hours (or days even) to track down. It also serves as very useful documentation to the reader. For example, if a subroutine takes two array arguments that should be of the same size, I'll do something like

subroutine foo (a, b)
  integer :: a(:), b(:)
  ASSERT(size(a) == size(b))
szaghi commented 8 years ago

@nncarlson Thank you very much! Very interesting.

muellermichel commented 8 years ago

@nncarlson: I like the ASSERT / INSIST macro idea a lot. makes me wonder, are there other macroes that could make sense in any FORTRAN project? Would it make sense to assemble a recommended macro library? One other thing that comes to mind is DPRINT (print to console only on NDEBUG).

SourcerersApprentice commented 8 years ago

On 1/20/2016 10:52 PM, Michel Müller wrote:

@nncarlson https://github.com/nncarlson: I like the ASSERT / INSIST macro idea a lot. makes me wonder, are there other macroes that could make sense in any FORTRAN project? Would it make sense to assemble a recommended macro library? One other thing that comes to mind is DPRINT (print to console only on NDEBUG).

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173476462.

I agree to this also. I frequently use Assert with C# in the default situation @nncarlson https://github.com/nncarlson describes, and it has saved me from some bug hunts. It would be great to standardize something like this in Fortran.

On the tension between writing clean code (no dead code) and keeping the noobs up to date, perhaps a comment signifying that implicit none is being inherited by use association would be appropriate? That clues the less informed that something unseen is going on while maintaining clean coding standards.

tclune commented 8 years ago

I picked this approach up from the ESMF project. Not sure who there suggested it, but it was likely Gerhard Theurich.

The idea is to put all of the optional dummy arguments for an interface after an argument of derived type that is private. This forces users of that interface to use keyword arguments for all optional arguments. This can catch certain types of mistakes in such contexts, but I mostly like it because it emphasizes the nature of those arguments in the client code.

ESMF used this technique to enforce backward compatibility. All new features were introduced with optional arguments (or entirely new procedures). Existing user code could not accidentally use the new features.

An example:

module Foo_mod implicit none private

public :: Foo

type :: Foo integer :: i contains procedure :: some_method end type

! This type is PRIVATE type :: Unusable end type

contains

subroutine some_method(this, unused, opt1, opt2) class (Foo), intent(inout) :: this type (Unusable), optional, intent(in) :: unused real, optional, intent(in) :: opt1 integer, optional, intent(in) :: opt2 … end subroutine some_method

end module Foo_mod

——————————————-

The refinement that I’ve not gotten around to doing yet is to put the Unusable type into its own module so that I don’t repeat it throughout a package. Of course, then some clever user might USE that module themselves, so maybe that is a bad idea?

tclune commented 8 years ago

Previously I had used ASSERT frequently in my code using the preprocessor. These days I tend use test driven development with a testing framework. As such the ASSERT’s represent “untestable” code because they actually stop processing. Instead, I’ve written a runtime “throw” method that uses a procedure pointer that can be configured to either STOP or send a signal to the testing framework. The default configuration is to STOP, but the initialization of the test framework toggles it to the other.

Over time though, I think I will start introducing preprocessor ASSERT’s in some instances. They are far less work than writing a procedure to watch for the exception. So unless the logic is particularly fragile/confusing, the ASSERT provides the important part of the safety net.

—————

I would like to add that there is interest in the Fortran standards committee in developing some programming-by-contract in the 2020 version of the standard. It is far too early to discuss particulars, nor can I even hazard a guess as to whether such a concept will make it to the short list. There is a general consensus that the committee needs to slow down to prevent large gaps between the standard and actual implementations by the vendors.

On Jan 21, 2016, at 1:52 AM, Michel Müller notifications@github.com wrote:

@nncarlson https://github.com/nncarlson: I like the ASSERT / INSIST macro idea a lot. makes me wonder, are there other macroes that could make sense in any FORTRAN project? Would it make sense to assemble a recommended macro library? One other thing that comes to mind is DPRINT (print to console only on NDEBUG).

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173476462.

Thomas Clune, Ph. D. Thomas.L.Clune@nasa.gov Software Infrastructure Team Lead Global Modeling and Assimilation Office, Code 610.1 NASA GSFC
MS 610.1 B33-C128 Greenbelt, MD 20771 301-286-4635

zbeekman commented 8 years ago

There is a general consensus that the committee needs to slow down to prevent large gaps between the standard and actual implementations by the vendors.

I would argue that the vendors need to get their $h!7 together! (At the risk of sounding like Linus Torvalds discussing Nvidia: the slow pace of development from some/most of the major compiler vendors has been pretty abhorrent. I'm not suggesting that their job is easy, but it seems like some of them have really been dragging their heels. And then when they do support a feature from a > 95 standard, it's half finished, bug riddled work. end rant)

cmacmackin commented 8 years ago

Well, in fairness, there is already support for some parts of the 2015 standard, so hopefully by 2020 all of the major vendors will have finally caught up. But yeah, things are kind of ridiculous at the moment. I can forgive gfortran, since its essentially a volunteer project, but the commercial compilers really have no excuse.

On 21/01/16 14:05, Izaak Beekman wrote:

There is a general consensus that the committee needs to slow down
to prevent large gaps between the standard and actual
implementations by the vendors.

I would argue that the vendors need to get their $h!7 together! (At the risk of sounding like Linus Torvalds discussing Nvidia: the slow pace of development from some/most of the major compiler vendors has been pretty abhorrent. I'm not suggesting that their job is easy, but it seems like some of them have really been dragging their heels. And then when they do support a feature from a > 95 standard, it's half finished, bug riddled work. |end rant|)

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173578571.

Chris MacMackin cmacmackin.github.io http://cmacmackin.github.io

milancurcic commented 8 years ago

@tclune While it seemed hackish, I always loved this feature/restriction in ESMF (except the few days that it took me to add keywords to all my ESMF calls) and thought it should be more adopted in large projects.

zbeekman commented 8 years ago

yes I'll add my :+1: for @tclune's ESMF requiring keywords for optional args technique. Makes code more legible if arguments are given sensible names and argument lists are not huge.

nncarlson commented 8 years ago

@tclune the ESMF keyword technique is very cool. I'd not seen this before, but I'm sure to use it now. I'd leave the definition of the unused type local and not use a module though; it's just several extra lines which I find preferable to introducing a new module dependency, new file, etc. I wonder if this isn't also a new language feature that could be proposed (if it hasn't already); a special argument list marker (e.g. ':') that indicates all subsequent arguments must use keywords? Off hand I don't see why such arguments must always be optional.

zbeekman commented 8 years ago

True, it appears you could force all arguments to use keywords by creating an inaccessible type and using it as the first dummy argument.

szaghi commented 8 years ago

Dear All,

I have added a first draft of this guideline. I hope to have added all in topic suggestions, for the off topic ones I have taken a note for other discussions (coming soon).

For the implicit templating trick I have added a comment here, but I realized during typing it that I have not completely understood it... please amend it with a more clear example, at least for me :smile:

See you soon.

milancurcic commented 8 years ago

@nncarlson @zbeekman

Off hand I don't see why such arguments must always be optional.

In the specific case of ESMF and @tclune correct me if I am wrong, the motivation behind this is to ensure future backward-compatibility. In this way, the API can be extended in the future versions while not breaking the procedure calls that were valid for previous versions of the library.

tclune commented 8 years ago

Yes - that was the ESMF motivation as I understood it at the time.

But it can only be used for requiring keywords for optional arguments. The reason is simply that the “Unusable” argument itself must be optional (think about it). And you cannot put non-optional arguments after optional arguments.

You could have a non-enforceable policy to always use keywords, but I would generally find that style to be overly verbose. Even modestly sized statements would tend to wrap beyond one line. But if your interfaces support small numbers of reasonably short argument names, I’d have no problem with it. The readers of that code would probably appreciate it.

On Jan 21, 2016, at 11:34 AM, Milan Curcic notifications@github.com wrote:

@nncarlson https://github.com/nncarlson @zbeekman https://github.com/zbeekman Off hand I don't see why such arguments must always be optional.

In the specific case of ESMF and @tclune https://github.com/tclune correct me if I am wrong, the motivation behind this is to ensure future backward-compatibility. In this way, the API can be extended in the future versions while not breaking the procedure calls that were valid for previous versions of the library.

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173626488.

Thomas Clune, Ph. D. Thomas.L.Clune@nasa.gov Software Infrastructure Team Lead Global Modeling and Assimilation Office, Code 610.1 NASA GSFC
MS 610.1 B33-C128 Greenbelt, MD 20771 301-286-4635

rouson commented 8 years ago

On Jan 21, 2016, at 5:21 AM, tclune notifications@github.com wrote:

There is a general consensus that the committee needs to slow down to prevent large gaps between the standard and actual implementations by the vendors.

I wish that slowing down would actually help vendors catch up, but I think there is more than one vendor the has stalled in the sense that they haven’t addd any significant new standards support in in quite a while. One vendor achieved Fortran 2003 compliance roughly 5 years ago and is still missing arguably the biggest Fortran 2008 feature: coarrays. Half a decade should have been enough time to implement the feature. Now amount of slowing down on the committee’s side will address this.

Another vendor finished Fortran 2003 compliance roughly 2 years ago and hasn’t added even one significant 2008 feature. In some arenas, this would lead to consolidation in the market, but it seems unlikely, given the various competitive advantages each vendor has on their own hardware (if they make hardware).

D

nncarlson commented 8 years ago

But it can only be used for requiring keywords for optional arguments. [...] And you cannot put non-optional arguments after optional arguments.

@tclune are you sure about that? A quick test with ifort and nagfor suggests it possible to have non-optional arguments follow optional arguments (in the procedure declaration). It is true that in a procedure reference non-keyword arguments are invalid after keyword arguments have been encountered, but that is exactly what is desired.

rouson commented 8 years ago

On Jan 21, 2016, at 6:05 AM, Izaak Beekman notifications@github.com wrote:

There is a general consensus that the committee needs to slow down to prevent large gaps between the standard and actual implementations by the vendors.

I would argue that the vendors need to get their $h!7 together! (At the risk of sounding like Linus Torvalds discussing Nvidia: the slow pace of development from some/most of the major compiler vendors has been pretty abhorrent. I'm not suggesting that their job is easy, but it seems like some of them have really been dragging their heels. And then when they do support a feature from a > 95 standard, it's half finished, bug riddled work. end rant)

I see that Zaak and I appear to be twins here of late. I just saw the above post after sending an almost identical message myself.

What is needed is for customers with deep pockets to make standards-requirement a contractual obligation for delivering major hardware. If a center buying a $100-million dollar supercomputer start stipulating Fortran 2008 compliance in their request for proposals for major acquisitions, then we would suddenly see Fortran 2008 compliance everywhere. This has happened and has worked on occasion. I suspect it would happen more often if users were louder. The Fortran community is a very quiet community. If any of you receive a user survey from a large supercomputer center, please respond and please make it very clear in the comments the Fortran 2008 compliance is pressing need for your project and tell everyone you know with accounts to do the same. At some point, the centers will have to take notice and the vendors will have to respond.

Just for little perspective, the Fortran 2003 standard was published in 2004 and the Fortran 2008 standard was published in 2010! Cray’s compiler has been Fortran 2008 compliant since at least 2014. The fact that no one has caught up with them or done the same thing now more than five years after the standard was published is a crime.

Damian

tclune commented 8 years ago

You are correct. I was typing quickly and rationalizing the answer that I expected. Of course, an optional argument before a non-optional argument is not really optional, is it?

On Jan 21, 2016, at 3:27 PM, Neil Carlson notifications@github.com wrote:

But it can only be used for requiring keywords for optional arguments. [...] And you cannot put non-optional arguments after optional arguments.

@tclune https://github.com/tclune are you sure about that? A quick test with ifort and nagfor suggests it possible to have non-optional arguments follow optional arguments (in the procedure declaration). It is true that in a procedure reference non-keyword arguments are invalid after keyword arguments have been encountered, but that is exactly what is desired.

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173698208.

Thomas Clune, Ph. D. Thomas.L.Clune@nasa.gov Software Infrastructure Team Lead Global Modeling and Assimilation Office, Code 610.1 NASA GSFC
MS 610.1 B33-C128 Greenbelt, MD 20771 301-286-4635

SourcerersApprentice commented 8 years ago

On 1/21/2016 12:29 PM, Damian Rouson wrote:

On Jan 21, 2016, at 6:05 AM, Izaak Beekman notifications@github.com wrote:

There is a general consensus that the committee needs to slow down to prevent large gaps between the standard and actual implementations by the vendors.

I would argue that the vendors need to get their $h!7 together! (At the risk of sounding like Linus Torvalds discussing Nvidia: the slow pace of development from some/most of the major compiler vendors has been pretty abhorrent. I'm not suggesting that their job is easy, but it seems like some of them have really been dragging their heels. And then when they do support a feature from a > 95 standard, it's half finished, bug riddled work. end rant)

I see that Zaak and I appear to be twins here of late. I just saw the above post after sending an almost identical message myself.

What is needed is for customers with deep pockets to make standards-requirement a contractual obligation for delivering major hardware. If a center buying a $100-million dollar supercomputer start stipulating Fortran 2008 compliance in their request for proposals for major acquisitions, then we would suddenly see Fortran 2008 compliance everywhere. This has happened and has worked on occasion. I suspect it would happen more often if users were louder. The Fortran community is a very quiet community. If any of you receive a user survey from a large supercomputer center, please respond and please make it very clear in the comments the Fortran 2008 compliance is pressing need for your project and tell everyone you know with accounts to do the same. At some point, the centers will have to take notice and the vendors will have to respond.

Just for little perspective, the Fortran 2003 standard was published in 2004 and the Fortran 2008 standard was published in 2010! Cray’s compiler has been Fortran 2008 compliant since at least 2014. The fact that no one has caught up with them or done the same thing now more than five years after the standard was published is a crime.

Damian

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173698724.

Having seen both sides here, both working for a vendor, and at other times being unable to afford the price of a compiler, I would remark that unless a vendor is married to a hardware manufacturer, they are probably getting squeezed pretty hard, and probably don't have the resources to do the required development. I think there is a high likelihood that as more people demand modern compiler features these vendors without connections will eventually be forced out.

I would also remark that the development of debugging capability is even more abysmal than compiler development. Many debuggers including gdb are unable to examine basic Fortran 90 data structures like linked lists, which have been in the language for some 25 years.

cmacmackin commented 8 years ago

There is a branch of GDB which can handle at least some of the F90 features, although I can't remember off the top of my head whether it can manage pointers. Also, which commonly used vendor (other than GFortran) isn't associated with a hardware manufacturer? NAG, I guess, but they aren't that big a player. On 22 Jan 2016 2:51 am, "SourcerersApprentice" notifications@github.com wrote:

On 1/21/2016 12:29 PM, Damian Rouson wrote:

On Jan 21, 2016, at 6:05 AM, Izaak Beekman notifications@github.com wrote:

There is a general consensus that the committee needs to slow down to prevent large gaps between the standard and actual implementations by the vendors.

I would argue that the vendors need to get their $h!7 together! (At the risk of sounding like Linus Torvalds discussing Nvidia: the slow pace of development from some/most of the major compiler vendors has been pretty abhorrent. I'm not suggesting that their job is easy, but it seems like some of them have really been dragging their heels. And then when they do support a feature from a > 95 standard, it's half finished, bug riddled work. end rant)

I see that Zaak and I appear to be twins here of late. I just saw the above post after sending an almost identical message myself.

What is needed is for customers with deep pockets to make standards-requirement a contractual obligation for delivering major hardware. If a center buying a $100-million dollar supercomputer start stipulating Fortran 2008 compliance in their request for proposals for major acquisitions, then we would suddenly see Fortran 2008 compliance everywhere. This has happened and has worked on occasion. I suspect it would happen more often if users were louder. The Fortran community is a very quiet community. If any of you receive a user survey from a large supercomputer center, please respond and please make it very clear in the comments the Fortran 2008 compliance is pressing need for your project and tell everyone you know with accounts to do the same. At some point, the centers will have to take notice and the vendors will have to respond.

Just for little perspective, the Fortran 2003 standard was published in 2004 and the Fortran 2008 standard was published in 2010! Cray’s compiler has been Fortran 2008 compliant since at least 2014. The fact that no one has caught up with them or done the same thing now more than five years after the standard was published is a crime.

Damian

— Reply to this email directly or view it on GitHub < https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173698724 .

Having seen both sides here, both working for a vendor, and at other times being unable to afford the price of a compiler, I would remark that unless a vendor is married to a hardware manufacturer, they are probably getting squeezed pretty hard, and probably don't have the resources to do the required development. I think there is a high likelihood that as more people demand modern compiler features these vendors without connections will eventually be forced out.

I would also remark that the development of debugging capability is even more abysmal than compiler development. Many debuggers including gdb are unable to examine basic Fortran 90 data structures like linked lists, which have been in the language for some 25 years.

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/Best_Practices/issues/10#issuecomment-173786992 .

muellermichel commented 8 years ago

I think that we should face one reality: Fortran is de facto today the language of a rich but niche market. Furthermore, as pointed out above, selling software has become increasingly hard, even for HPC. Paying HP 100 million? No problem. Paying PGI, Allinea and Roguewave 10 million (i.e. 20 person years if it by miracle all went into R&D, realistically more like 5 years)? Suddenly a problem. The only ones who've basically fixed this are Cray, by basically being the Apple of HPC (vertical integration). And indeed their compilers tend to be among the most compliant, at least AFAIK.

So my point being: We simply have to live with the fact that 2008 compliance is still way out for some compilers. The question is, whether portability is more important to the best practices proposed here, or whether it is "modernness" of code. I tend to favor portability. It would be interesting to get the other opinions on this.

Edit: One more idea on this: How about we add some visual markers to the Guide that displays the compiler compatibility for each recommendation? Maybe format it as a html table with unicode symbols? [cm] = checkmark


[cm] All Compilers

[cm] GNU [x] g95 [x] ifort [x] PGI [cm] Cray [x] IBM