fortran-lang / stdlib

Fortran Standard Library
https://stdlib.fortran-lang.org
MIT License
1.05k stars 164 forks source link

Style guide #3

Open milancurcic opened 4 years ago

milancurcic commented 4 years ago

Use this issue to discuss the code style for the stdlib.

The most widely supported elements of style will eventually be merged into the Style Guide for contributors.

certik commented 4 years ago

@ivan-pi wrote in https://github.com/fortran-lang/stdlib/issues/11#issuecomment-566296346:

For example for the function that checks whether a character is a letter some of the possible names are:


I have discussed this particular issue with several people many years ago and we eventually converged towards the following compromise:

https://www.fortran90.org/src/best-practices.html#naming-convention

So in the above case, isAlpha would not be used, but both is_alpha and isalpha would be allowed, and in this case isalpha would be preferred (only two syllables). If the name was is_char_alpha vs ischaralpha, then the underscore version would be preferred because it is three syllables. However, if the API had lots of methods like is_alpha, is_char_alpha, is_not_alpha, then for consistency it should be named is_* with underscore.

Let me know what you think.

P.S. Some examples from current Fortran. No underscore: matmul, maxloc, minloc, iachar, adjustl, adjustr, maxexponent, minexponent, rrspacing, ishftc. Underscore: set_exponent, dot_product, bit_size, cpu_time. The standard is not consistent (e.g., maxexponent vs set_exponent) but it seems to be very close to the rule of 2 syllables -> no underscore, otherwise use underscore (e.g. matmul vs dot_product).

ivan-pi commented 4 years ago

isalpha contains three syllables.

In general I agree with your suggested naming conventions and also the ones in Fortran Best Practices. To stay close to the standard, I think it's best if we avoid CamelCase.

Specifically, for the ascii functions there are 11 functions starting with is, some are short (is_alpha), but some are long (is_hex_digit, is_octal_digit). I suppose it's easiest to just follow the C library and get rid of the underscores and shorten the names to 7-8 characters (isalpha, iszdigit - z is used in hexadecimal boz constants, isodigit - o is used in octal boz constants).

certik commented 4 years ago

You are right, isalpha is 3 syllables.

I think the rules can be broken where it makes sense and we have a good reason. Otherwise if we have no reason then the rules provide a good consistent default.

On Mon, Dec 16, 2019, at 5:18 PM, Ivan wrote:

isalpha contains three syllables.

In general I agree with your suggested naming conventions and also the ones in Fortran Best Practices https://github.com/Fortran-FOSS-Programmers/Best_Practices. To stay close to the standard, I think it's best if we avoid CamelCase.

Specifically, for the ascii functions there are 11 functions starting with is, some are short (is_alpha), but some are long (is_hex_digit, is_octal_digit). I suppose it's easiest to just follow the C library https://en.cppreference.com/w/c/string/byte and get rid of the underscores and shorten the names to 7-8 characters (isalpha, iszdigit - z is used in hexadecimal boz constants, isodigit - o is used in octal boz constants).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/fortran-lang/stdlib/issues/3?email_source=notifications&email_token=AAAFAWE3XEAVNGIXF5BNKJLQZALGHA5CNFSM4J25FZ62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHAT2DY#issuecomment-566312207, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAFAWCAIOCRTWMOKJEVFX3QZALGHANCNFSM4J25FZ6Q.

milancurcic commented 4 years ago

I'm on the same page here. I like all lowercase with underscores universally. For short variable and procedure names, dropping the underscore is OK I think. It's nice to have a universal rule to follow (always use underscores to separate words), but for some short names I think it's OK to make an exception.

I don't think the Standard being inconsistent (maxexponent and set_exponent) should be an excuse for us to be "consistently inconsistent". There's also a difference between names that are made of whole words (e.g. dot_product is nicer than dotproduct), vs. portmanteaus like matmul (instead of mat_mul which seems awkward).

In this specific case, even though iszdigit and isodigit are short and nice, there's a risk of them being less clear what they mean. To my eye, is_hex_digit and is_octal_digit were obvious. For iszdigit and isodigit, I had to read Ivan's explanation to understand. So I slightly prefer the underscore variants here, but I'm not hell-bent on it.

zbeekman commented 4 years ago

Yes, I generally agree but for assertion-y stuff I think separating the verb with an underscore makes sense is_alpha. The legibility is much improved and client code can always consume it as a different name through use association.

use fortran_stdlib, only: is_alpha => isalpha
zbeekman commented 4 years ago

Automation of pedantry

I think that we should also adopt solutions like Editorconfig, possibly findent cmake-format (if we use CMake) and clang-format (for any C code). Tools like these that integrate with IDEs and editors will help get everyone on the same page allowing them to focus on semantics and code rather than style and formatting. In addition, I've really been appreciating and enjoying pre-commit to codify conventions on a per-project basis, and help catch silly issues before developers even commit/push code.

certik commented 4 years ago

Yes, I agree with @zbeekman we should try to automate as much as possible (formatting, release notes, etc.).

And rather use our time and effort to discuss the actual API, not how it is formatted.

I agree that is_alpha looks better in this case. I would caution against recommending use fortran_stdlib, only: is_alpha => isalpha, because that is a pain to have to do that --- rather, let's do our best to choose the name well, and recommend people to use it. That way all the codes will use the same name and it will be easy for people reading the code to understand.

smwesten-usgs commented 4 years ago

In this specific case, even though iszdigit and isodigit are short and nice, there's a risk of them being less clear what they mean. To my eye, is_hex_digit and is_octal_digit were obvious. For iszdigit and isodigit, I had to read Ivan's explanation to understand. So I slightly prefer the underscore variants here, but I'm not hell-bent on it.

Should I get a vote, I also prefer more verbose but hopefully clearer names, and prefer lowercase and underscores universally: is_hex_digit > iszdigit is_octal_digit > isodigit

Regarding names of string functions/subroutines, I'd definitely prefer more verbose and explicit over the short and cryptic. Thus: as_integer > atoi as_double > atof

certik commented 4 years ago

@smwesten-usgs thanks for the feedback. Yes, I agree on the examples you gave that underscores look better.

Here are some examples where no underscores I think look better (from NumPy and Matlab):

longb commented 4 years ago

Just speculation, but I suspect the difference between the underscore usage between maxexponent and set_exponent is that max is already the name of a different intrinsic function.

zbeekman commented 4 years ago

I agree, and for the record, I wasn't suggesting we pick whatever and let people rename through use association; Good names from the get-go are important.

zbeekman commented 4 years ago

Can we make some choices about indentation and tabs vs spaces? My preference is indent all indent-able things by two spaces; never use tabs. We can codify this in an editor-config file.

certik commented 4 years ago

My preference is 4 spaces, no tabs.

zbeekman commented 4 years ago

A quick note: I'm happy with 2 or 4 but with the 132 character line limit, you can start to run out of real estate pretty quickly with 4 spaces.

certik commented 4 years ago

I also propose to stick to 80 characters per line.

More importantly though we should figure out automatic formatting checks at the CI and provide instructions how developers can format the code before submitting a PR.

On Sun, Dec 22, 2019, at 10:45 AM, zbeekman wrote:

A quick note: I'm happy with 2 or 4 but with the 132 character line limit, you can start to run out of real estate pretty quickly with 4 spaces.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/fortran-lang/stdlib/issues/3?email_source=notifications&email_token=AAAFAWCALED2B7ENIAUQTPLQZ6RUFA5CNFSM4J25FZ62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHPV5RY#issuecomment-568286919, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAFAWDCAOJSSZM3QLGLKE3QZ6RUFANCNFSM4J25FZ6Q.

zbeekman commented 4 years ago

80 characters per line and 4 spaces?! That seems like there will be an awful lot of line continuations and leading blanks... findent defaults to 2 spaces. The standard caps line length at 132 chars. I think we should shorten the line length allowed XOR pick 4 spaces of indentation, but I'd be weary to use both at once.

certik commented 4 years ago

Yes, I've been using 80 characters and 4 spaces in all my projects.

But whatever we decide is fine with me. Here are my priorities for stdlib:

How we format the files are low on my priorities list, so whatever makes us work together works for me.

ivan-pi commented 4 years ago

I generally use 4 spaces (same as Python and most C++ codes I've seen), but I also don't mind if we agree to use 2 and I agree with the points of @certik. Generally, I try to stick to an 80 character limit, but I am happy to break the rule if it makes the indentation nicer.

zbeekman commented 4 years ago

OK, let's go with 4 then, and we could admonish users to be within 80 characters (or 100, or whatever) but enforce a hard limit of 132 during CI. I'm working on getting a style document updated with comments from here and an .editorconfig file as well as CI testing and enforcement.

longb commented 4 years ago

I generally use whatever the number of spaces created by emacs when I hit "tab". Note that the line length limit will probably get really long (10,000 characters) in the next standard, so debates over 80 versus 132 are a bit dated. Personally I prefer to be able to read a line of code in a terminal window without horizontal scrolling. But I think that most "style guides" include a lot of unnecessary "nanny" rules that don't matter that much. I think a rule like "a sequence of lines that form a block in Fortran should be indented compared to the code lines that delineate the block" is sufficient. Much more relevant (in my view) would be code style rules such as "do not use include lines", or "never use preprocessing directives", or "never use tools like configure, cmake, or spack" would be more useful and relevant to Fortran.

zbeekman commented 4 years ago

@longb haha "never use tools like ... cmake"

Sometimes there are necessary evils.

I agree about reading things in the terminal. Thats the spirit of creating a limit.

Concerning emacs, it's easy to setup to respect .editorconfig.

Here is what I have in my .emacs (I use use-package)

(use-package editorconfig
  :ensure t
  :after ws-butler
  :init
  (setq editorconfig-trim-whitespaces-mode
         'ws-butler-mode)
  :config
  (editorconfig-mode 1)
  ;; Always use tabs for Makefiles
  (add-hook 'editorconfig-hack-properties-functions
        '(lambda (props)
           (when (derived-mode-p 'makefile-mode)
         (puthash 'indent_style "tab" props)))))
zbeekman commented 4 years ago

I've started trying to capture this discussion in PR #42

milancurcic commented 4 years ago

I prefer 2 spaces to indent, and try (but sometimes fail) to stay within 80-character line length. This is not for compatibility reasons, but for the same reason as Python recommendation -- I work in terminals of similar width and don't like the lines to wrap around.

I think consistency is more important than any specific style "rule". I also don't think we need style enforcement (just yet). A brief style guide with recommendations and common sense go a long way.

longb commented 4 years ago

Some style suggestions that are probably obvious, but seem to be violated in some user code:

1) Do not use language features that have been deleted (PAUSE, Arithmetic IF, ...). 2) Avoid using language features that are obsolescent (such as COMMON). 3) Avoid vendor syntax extensions (such as REAL*8), especially when there are standard alternatives. 4) Avoid vendor-specific, nonstandard intrinsic procedures (such as ETIME).

Violating these leads to portability problems.

milancurcic commented 4 years ago
  1. Do not use language features that have been deleted (PAUSE, Arithmetic IF, ...).
  2. Avoid using language features that are obsolescent (such as COMMON).
  3. Avoid vendor syntax extensions (such as REAL*8), especially when there are standard alternatives.
  4. Avoid vendor-specific, nonstandard intrinsic procedures (such as ETIME).

I agree. We can express these in the style guide as simply as:

milancurcic commented 4 years ago

@certik I'm looking at some merged code and I now understand why you're okay with 4 spaces to indent and 80 character line limit: You don't indent module, program, and procedure bodies. I think we should.

I like 2 space-indents but can work with 4 spaces as well. But I think we should indent all program unit bodies consistently. The recommendation can then be applied to indenting bodies of all Fortran constructs:

certik commented 4 years ago

Yes, indeed, I don't indent subroutines. The reason for that is that if you do, then every single subroutine is indented by 4 spaces. That's unfortunate, as that is lost space. Just like in C++, if you use namespaces, you do not indent the functions inside namespaces for the same reason. The indentation does not buy you anything. You know that every subroutine is inside a module. And module is like a C++ namespace with this respect.

Inside the functions bodies, I also don't indent (I don't think it buys you any readability and you lose horizontal space), but if you feel strongly about it, then we can.

Doing 2 spaces for subroutines and 4 spaces for everything else is pretty complicated --- I would need to figure out how to setup my Vim editor to support that. Doing 4 spaces everywhere then risks of running out of the 80 character limit more often, precisely as @zbeekman was worried.

But since we relaxed the 80 char limit rule, I think it's fine to use 4 spaces everywhere.

Overall, this is relatively low priority for me, so I am happy to do whatever you feel strongly about. Let's setup tools to do the formatting automatically though --- that I feel strongly about. :)

milancurcic commented 4 years ago

I like indenting everything, but it'd be a stretch to say I feel strongly about it. It does help readability for me, but only because my eyes are attuned to it.

Let's hear from others and hopefully the decision will be easy.

@marshallward @jacobwilliams @zbeekman @ivan-pi @jvdp1 @longb @smwesten-usgs Where are you on indenting bodies of program units?

certik commented 4 years ago

What we can do is to choose something reasonable (anything we discussed above is reasonable for me) and setup automatic formatting. Then as we have more code, we can compare different formatting options side by side and we can change the default formatting by changing some configuration option in the formatting tool if we like one option better as a community.

marshallward commented 4 years ago

MOM6 does two-space indents.  My personal preference is 4 spaces since 2-space indent "errors" are harder to detect visually and can slip in more easily.  But I haven't noticed much of an issue using 2-space in MOM6.

We do not indent a module's contents, as long as we stick to one module per file.  In this interpretation, the model statement/ending, the contains, etc are more like declarations than indications of scope.  But these rules might not make sense if there were multiple modules per file.

Otherwise, I'd say everything that moves into a more narrow scope is indented: subroutine contents, type definitions, control blocks, etc.

I wouldn't want to deal with mixed rules, I'd say either 2 or 4 spaces for everything.

I am personally militant about 80-character limits, since I usually resize my fonts to fit about ~190 characters, or ~85 on two parallel terminals. MOM6 "encourages" 100, with a hard limit on 120. This works well overall (for everyone except me and my font sizes, that is!)

For all the various edge cases, like how to split binary operators, I tend to follow PEP8. But that mostly reflects my undying love for Python.

As an acquaintance of mine says: "If your code only looks good under a particular style, then I probably won't like reading your code anyway".  As long as things can be broken up into modular pieces, then it ought to look good under 2-, 4- or even 8-space indents.

PS if anyone wants to see the (incomplete) MOM6 style guide.

(And for the record I do not necessary agree with all of these rules!)

jvdp1 commented 4 years ago

Where are you on indenting bodies of program units?

I use 1-space for indenting subroutines/functions, but also interfaces, derived types,... I am fine with 2-space indents, and 80 (soft)-/132(hard)-character limits. 4-space indents seems a bit too much to me, but it would not be a problem neither.

ivan-pi commented 4 years ago

Where are you on indenting bodies of program units?

I tend to indent everything (besides for contain statements which stay parallel to the enclosing module/type).

Since we plan to have a single file per module I can agree with @certik and @marshallward that indentation at module level would save some horizontal space. However I would prefer to indent subroutine/function bodies (like in Python). I think it helps to visually separate the functions/subroutines bodies from each other without the need for comment lines such as

!-------------------------------------------

which I find are just a nuisance to create and maintain.

gronki commented 4 years ago

If I can have an annoying remark, I don't feel about indentations as strongly as I do about using spaces. For example,

if(s%value==3)error stop

Is nightmare to read compared to

if (s % value == 3) error stop

And since someone will likely disagree with this too, I would like suggest that people will be always unhappy with how things are indented and spaced out. Even here everyone has expressed a different opinion. I personally would only require sane indentation and not using tabs.

śr., 25 gru 2019, 16:58 użytkownik Ivan notifications@github.com napisał:

Where are you on indenting bodies of program units?

I tend to indent everything (besides for contain statements which stay parallel to the enclosing module/type).

Since we plan to have a single file per module I can agree with@certik and @marshallward https://github.com/marshallward that indentation at module level would save some horizontal space. However I would prefer to indent subroutine/function bodies (like in Python). I think it helps to visually separate the functions/subroutines bodies from each other without the need for comment lines such as

!-------------------------------------------

which I find are just a nuisance to create and maintain.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/fortran-lang/stdlib/issues/3?email_source=notifications&email_token=AC4NA3I6PPIGWTRVTVICHGTQ2N7LLA5CNFSM4J25FZ62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHUOLAI#issuecomment-568911233, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC4NA3KC3D2X74KZ26SJBALQ2N7LLANCNFSM4J25FZ6Q .

zbeekman commented 4 years ago

Distilling the above comments down, and adding my own opinion it appears that:

  1. Most people want procedure bodies indented (I do too)
  2. Most people are OK with a "shoot for 80 chars, but hard limit of 132" line length
  3. With only one program or module per file, indenting these scopes does eat up blank space. With an indentation level of 4 this seems to be a bigger issue. While I agree that it isn't really necessary to indent these scopes if we adopt the convention of 1-per-file, from a tooling/enforcement perspective most tools, editors, etc. that I've seen want to indent these scopes by default.

Takin points 1-3 into consideration, as well as the comments expressed thus far, I would be inclined to indent two (2) spaces for all procedures and module and program scopes too. I'll go ahead and implement this in the EditorConfig file and style guide I've started in #42.

I would also recommend that we take this discussion over to the PR (#42) and that people make suggestions of how and where they would like to see the ideas discussed in this issue expressed. I've started working on a style guide document, and if you have expressed an opinion in this issue, you should look at #42 to ensure that the document is acceptable and that your opinion or suggestion has been sufficiently addressed.

urbanjost commented 4 years ago

For all documentation I try to always include a complete working example program. I always call it "program demo_NNNNN" where NNNN is the name of the routine being tested, so it is easy to automate extracting and running the sample code, and to extract the code into a directory of small programs for use by users of the procedure. This lets the developer easily test code supplied in the documentation, and prevents simple syntax errors and problems like I found in the documentation for the experimental open(3f) routine:

program test
    use stdlib_experimental_stats, only: mean
    implicit none
    integer :: io, u
    u = open('example.dat', 'rt', iostat = io)
end program

So as to facilitate something like that in the long term, this should start with "program demo_open" and end with "end program demo_open". If all the steps were implemented the typo in the above would be automatically caught. In the mean time when writing documentation you could use something like

man -s $SECTION $TOPIC| col -b| expand| sed -n -e '\%^[ !]program demo%,\%^[ !]end program *demo%{p}' >$FILE

which is a line extracted from an automatic tester of syntax and/or whether a demo routine can be compiled and loaded in manpages. If would be easy to change this to handle multiple demos in a single markdown file in the long term.

PS: In the sample program in the document it is OPEN(3f) that should be added to the local scope, not the MEAN(3f) function in stdlib_experimental_io.md.

jvdp1 commented 4 years ago

@urbanjost Thank you for spotting the errors and for the suggestions. I support this idea of an automati tester. Maybe the automatic tester could be also added to CMake and the CI? @certik would it be possible for the CI? I also think it would be good to add a template of a spec. The spec for open was a first attempt. and suggestions as @urbanjost ones can be useful.

certik commented 4 years ago

Yes, I would recommend to run documentation tests. We can call them doctests, just like in Python.

LFortran could eventually be used to run those naturally. Until then, I suggest we append end and compile + run manually using gfortran.

fiolj commented 4 years ago

In order to have a consistent code style, could we advice to use a formatting tool with the specified standard? For instance, currently I am using fprettify (https://github.com/pseewald/fprettify) with some options (in my case fprettify -w 3 -i 2) wich gives two spaces indentation, but any tool would be useful.

certik commented 4 years ago

Yes, the plan is to use a tool and enforce it by the CI. @zbeekman do you still plan to do work on that?

aradi commented 4 years ago

Line length should be chosen IMHO by considering, what is the longest line length you can easily still scan with your eyes. I made good experience with 80 and 100 (preferred) chars per line. For me, everything beyond 100 is hard to read.

For the indentation: I think, it is most consistent to require, that whenever you open a scope / block construct, you increase indentation. That means, all subroutines within the module-construct would be indented. I think, most editors handle that this way anyway, so that would be easiest for the contributors.

Finally, 2 or 4 chars for indentation. If we are really going to use a preprocessor for generating specifics from a template, the according constructs should be also indented to enhance the readability of the generic code (the one most programmers would have to deal with):

#:include "common.fypp"
#:set RANKS = range(1, MAXRANK + 1)

submodule (stdlib_experimental_stats) stdlib_experimental_stats_mean
  use stdlib_experimental_error, only: error_stop
  implicit none

contains

  #:for k1, t1 in REAL_KINDS_TYPES
    #:for rank in RANKS
      module function mean_${rank}$_all_${k1}$_${k1}$(x) result(res)
        ${t1}$, intent(in) :: x${ranksuffix(rank)}$
        ${t1}$ :: res

        res = sum(x) / real(size(x, kind = int64), ${k1}$)

      end function mean_${rank}$_all_${k1}$_${k1}$
    #:endfor
  #:endfor

 [...]

end submodule

versus

#:include "common.fypp"
#:set RANKS = range(1, MAXRANK + 1)

submodule (stdlib_experimental_stats) stdlib_experimental_stats_mean
    use stdlib_experimental_error, only: error_stop
    implicit none

contains

    #:for k1, t1 in REAL_KINDS_TYPES
        #:for rank in RANKS
            module function mean_${rank}$_all_${k1}$_${k1}$(x) result(res)
                ${t1}$, intent(in) :: x${ranksuffix(rank)}$
                ${t1}$ :: res

                res = sum(x) / real(size(x, kind = int64), ${k1}$)

            end function mean_${rank}$_all_${k1}$_${k1}$
        #:endfor
    #:endfor

 [...]

end submodule

So, my 2 cents would be:

certik commented 4 years ago

I think @zbeekman is busy.

@fiolj or @aradi would you be willing to setup automatic enforcing of the formatting at our CI? That would be super helpful.

longb commented 4 years ago

I find the proposed 2 space indentation and 100 character line limit very reasonable.

The foreign preprocessing notation in the example is horrifying. There is a template proposal before WG5 that would make this sort of awkwardness unnecessary. In general, a coding style document for Fortran should include a hard prohibition on any sort of preprocessing.

Cheers, Bill

On Jan 29, 2020, at 3:28 AM, Bálint Aradi notifications@github.com wrote:

Line length should be chosen IMHO by considering, what is the longest line length you can easily still scan with your eyes. I made good experience with 80 and 100 (preferred) chars per line. For me, everything beyond 100 is hard to read.

For the indentation: I think, it is most consistent to require, that whenever you open a scope / block construct, you increase indentation. That means, all subroutines within the module-construct would be indented. I think, most editors handle that this way anyway, so that would be easiest for the contributors.

Finally, 2 or 4 chars for indentation. If we are really going to use a preprocessor for generating specifics from a template, the according constructs should be also indented to enhance the readability of the generic code (the one most programmers would have to deal with):

:include "common.fypp"

:set RANKS = range(1, MAXRANK + 1)

submodule (stdlib_experimental_stats) stdlib_experimental_stats_mean use stdlib_experimental_error, only: error_stop implicit none

contains

:for k1, t1 in REAL_KINDS_TYPES

#:for rank in RANKS
  module function mean_${rank}$_all_${k1}$_${k1}$(x) result(res)
    ${t1}$, intent(in) :: x${ranksuffix(rank)}$
    ${t1}$ :: res

    res = sum(x) / real(size(x, kind = int64), ${k1}$)

  end function mean_${rank}$_all_${k1}$_${k1}$
#:endfor

:endfor

[...]

end submodule

versus

:include "common.fypp"

:set RANKS = range(1, MAXRANK + 1)

submodule (stdlib_experimental_stats) stdlib_experimental_stats_mean use stdlib_experimental_error, only: error_stop implicit none

contains

#:for k1, t1 in REAL_KINDS_TYPES
    #:for rank in RANKS
        module function mean_${rank}$_all_${k1}$_${k1}$(x) result(res)
            ${t1}$, intent(in) :: x${ranksuffix(rank)}$
            ${t1}$ :: res

            res = sum(x) / real(size(x, kind = int64), ${k1}$)

        end function mean_${rank}$_all_${k1}$_${k1}$
    #:endfor
#:endfor

[...]

end submodule

So, my 2 cents would be:

• Indentation by 2 chars, consistently whenever a block construct is opened. • Max. line length: 100 chars. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Bill Long longb@cray.com Principal Engineer, Fortran Technical Support & voice: 651-605-9024 Bioinformatics Software Development fax: 651-605-9143 Cray, a Hewlett Packard Enterprise company/ 2131 Lindau Lane/ Suite 1000/ Bloomington, MN 55425

certik commented 4 years ago

@longb thanks for the feedback.

Indeed, the pre-processing is not super readable. However, the other alternatives that we considered are even worse. We should strive to include enough features into Fortran so that this kind of pre-processing will become unnecessary, but that's for the long term.

fiolj commented 4 years ago

@fiolj or @aradi would you be willing to setup automatic enforcing of the formatting at our CI? That would be super helpful.

I never used nor modified any CI in my projects, and do not know how to work with EditorConfig yet, so if somebody else take this task would be better. Otherwise, I'll start learning...

aradi commented 4 years ago

The foreign preprocessing notation in the example is horrifying. There is a template proposal before WG5 that would make this sort of awkwardness unnecessary. In general, a coding style document for Fortran should include a hard prohibition on any sort of preprocessing.

@longb I absolutely agree, pre-processing is evil! But it is IMHO even more evil, that we are forced to use pre-processing due to the lack of an appropriate feature in the language.

Would you mind to take a look, as an example, on the implementation of the mean() function in this library, which allows the reduction of the rank of an arbitrary multi-dimensional array by one. Do you see any better solution without pre-processing which avoids duplicating basically the same code 100-times and which is available now (and not in 10-20 years)?

aradi commented 4 years ago

@certik I could try to have a look at it (although I am not experienced with the CI in GitHub), but not earlier than end of next week...

urbanjost commented 4 years ago

For cases where the preprocessor is not being used for conditional selection of a programming environment but to expand a template or allow for single-file source with documentation and unit tests and so on the distribution should include the expanded source. Requiring a specific development environment to be available for someone to just build and use the code is very unattractive. I keep virtually all my code in a preprocessor file format and use a very heavily customized and automated build environment but if I put something out for public consumption I put out standard .f90 .c ... files and an (automatically generated) makefile with some cpp(1) directives (if conditionals are absolutely necessary). It's one thing to expect a developer to have to have fypp, cmake, doxygen, ... installed, but very disturbing that someone that wants to use the results is required to install a full development environment. Ideally one should have an expanded directory in the distribution that you need nothing but a Fortran compiler to build from.

On another note, since for the integer versions of mean(3f) everything is promoted to a REAL type you could write a single routine with a CLASS(*) argument and a short select type instead of a template which might not be quite as efficient but would be much simpler to maintain. so there are alternatives in some cases to a template.

jvdp1 commented 4 years ago

For cases where the preprocessor is not being used for conditional selection of a programming environment but to expand a template or allow for single-file source with documentation and unit tests and so on the distribution should include the expanded source. Requiring a specific development environment to be available for someone to just build and use the code is very unattractive. I keep virtually all my code in a preprocessor file format and use a very heavily customized and automated build environment but if I put something out for public consumption I put out standard .f90 .c ... files and an (automatically generated) makefile with some cpp(1) directives (if conditionals are absolutely necessary). It's one thing to expect a developer to have to have fypp, cmake, doxygen, ... installed, but very disturbing that someone that wants to use the results is required to install a full development environment. Ideally one should have an expanded directory in the distribution that you need nothing but a Fortran compiler to build from.

In addition to have the templated code on Github, it has already been suggested by @certik in #35 comment to provide release tarballs that contain all necessaty generated files to the users. I think it is the way to go (i.e., providing templated codes, and generated tarballs). We just need someone to implement it ;)

On another note, since for the integer versions of mean(3f) everything is promoted to a REAL type you could write a single routine with a CLASS(*) argument and a short select type instead of a template which might not be quite as efficient but would be much simpler to maintain. so there are alternatives in some cases to a template

I think it is easier to maintain if the mean function for integer is (almost) the same as the mean function for real. You only need to think about one structure, but it is maybe only my way of thinking.

aradi commented 4 years ago

@urbanjost The plan is to "ship" the library with the preprocessed proper Fortran standard files. The repository, however, should contain the sources with the un-preprocessed files. Otherwise, you get crazy with diffs, if you have any changes in those routines. What we could consider is to add the preprocessor (1 single file) to the repository, so having Python (which is quite common) would be enough to build the library. (I have created #133 on this.)

As for class(*): It deserves maybe a separe issue as well. But what you do then, is making the code repetition on a lower lever (inside the subroutine instead of repeating the subroutine). But you still have to make a case for each possible data type there, and do something different (but almost identical) for each rank to be considered as well. So, currently I don't see a way to avoid code repetition, and if we have to repeat code ca. 50-times with minimal changes, I still prefer the ugly preprocessor-trick.

urbanjost commented 4 years ago

I find I have a number of routines where it is desirable or tolerable to always promote to doubleprecision. An elemental routine that takes any scalar and returns a double is called. There are pros and cons but it is very useful in practice for me. For example if you had something like

pure elemental function anyscalar_to_double(valuein) result(d_out)
use, intrinsic :: iso_fortran_env, only : error_unit !! ,input_unit,output_unit
implicit none
!anyscalar_to_double(3f): convert integer or real parameter of any kind to doubleprecision
class(*),intent(in)       :: valuein
doubleprecision           :: d_out
doubleprecision,parameter :: big=huge(0.0d0)
   select type(valuein)
   type is (integer(kind=int8));   d_out=dble(valuein)
   type is (integer(kind=int16));  d_out=dble(valuein)
   type is (integer(kind=int32));  d_out=dble(valuein)
   type is (integer(kind=int64));  d_out=dble(valuein)
   type is (real(kind=real32));    d_out=dble(valuein)
   type is (real(kind=real64));    d_out=dble(valuein)
   type is (real(kind=real128))    d_out=dble(valuein)
   class default
     d_out=nan(d_out)
     !!stop '*M_anything::anyscalar_to_double: unknown type'
   end select
end function anyscalar_to_double

then other routines have an argument of class() and call this routine to convert it to a double. I use templating only where for optimization reasons it is justified. For a public library where anyone might be using it for anything templating is fine in lieu of Fortran features that support something similiar; but class() can be used very effectively and makes the code very easy to maintain.

Not a panacea but very useful for cases it is a fit for.