Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore preprocessor lines when detecting fixed form fortran #302

Merged
merged 3 commits into from
Jan 21, 2024

Conversation

albertziegenhagel
Copy link
Contributor

Currently a file with the content

#if defined(A) && !defined(B)
c This is a fixed-form style comment
#endif

is categorized as free-form Fortran because the preprocess if-clause contains ampersands and exclamation marks.

This commit makes the detection algorithm ignore all lines starting with # if pre-processing is enabled for that file. Pre-processor lines are not valid Fortran code and should IMO not be taken into account when trying to determine the Fortran form.

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (71d5f40) 87.51% compared to head (f118412) 87.52%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #302      +/-   ##
==========================================
+ Coverage   87.51%   87.52%   +0.01%     
==========================================
  Files          35       35              
  Lines        4756     4760       +4     
==========================================
+ Hits         4162     4166       +4     
  Misses        594      594              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@albertziegenhagel
Copy link
Contributor Author

albertziegenhagel commented Jun 29, 2023

I have added a changelog entry for this under "Changed". Let me know if you would rather consider this a "fix" (or anything else) and I will adjust it accordingly.

Edit: and if you would prefer that I open an issue for this PR first and use the issue link instead of the PR link in the changelog, let me know about that as well and I will open one.

@gnikit
Copy link
Member

gnikit commented Jun 29, 2023

@albertziegenhagel Are you sure that preprocessor directives in fixed form are standard conforming?
It would still be a valid addition even if it wasn't, but I haven't seen fixed form code using preprocessor directives.

@albertziegenhagel
Copy link
Contributor Author

I might not understand the question correctly, but to the best of my knowledge C style preprocessor directives are not standard in any Fortran form. It's just what people do and "all" compilers support it.

At least in our code base it is quite common to have pre-processed fixed-form Fortran files with .F ending. And fortls already distinguishes between .f and .F correctly and applies pre-processing to those files.

But I would not consider myself an expert on Fortran, so I might be missing something.

On another note, it might be a good idea to use the file extension to determine whether a file should be parsed in fixed-form or free-form (.f and .F for fixed-form, .f90, .F90, .f03, ... for free-form). I think that is what most (all?) compilers do. And we could fallback to the automatic detection for files with other extensions than the common ones. But this is probably somewhat orthogonal to this PR.

@gnikit
Copy link
Member

gnikit commented Jun 30, 2023

I might not understand the question correctly, but to the best of my knowledge C style preprocessor directives are not standard in any Fortran form. It's just what people do and "all" compilers support it.

Yeah, you are correct, I had completely forgotten about preprocessing + Fixed Form. The Standard only defines what a Fortran processor (i.e. a compiler) should do. Preprocessing as the name suggests is outside of the Standard's scope.
I will note that there are nuances between compiler vendors and how each has implemented their Fortran pre-processor, but let's not worry about that too much right now.

On another note, it might be a good idea to use the file extension to determine whether a file should be parsed in fixed-form or free-form (.f and .F for fixed-form, .f90, .F90, .f03, ... for free-form). I think that is what most (all?) compilers do. And we could fallback to the automatic detection for files with other extensions than the common ones. But this is probably somewhat orthogonal to this PR.

The problem is that most compilers use file extensions heuristics, BUT these heuristics are vastly different between compilers vendors and can even clash with one another. Also, compilers offer flags to explicitly override these heuristics, because for many cases they just don't work.

See this issue by Ivan fortran-lang/fpm#250 that lists the most common extensions, compilers combinations in a nice table and this very lengthy (and now closed) discussion fortran-lang/fpm#363 we have had about file extensions, compilers and possible changes.

So in terms of heuristics the most robust way to understand what form a file is, is by inspecting the comment character. That won't always work since you can have mixed (Fixed + Free) Fortran code, but in most cases it does remove the burden from the user to have to toggle options and flags to fortls when they write code. So an extension-based heuristic for detecting form in fortls is unlikely to be implemented.

@albertziegenhagel
Copy link
Contributor Author

Thanks a lot for the links to the discussions in fpm!

I guess you are completely right that under these circumstances it wouldn't make sense to use the file extension to detect fixed/free form Fortran (at least as long as one does not know which compiler is to be used by the project, and even then all used compilers would have to agree). I am wondering whether we can provide an easy way to overwrite the heuristics, similar to how compilers allow to pass -ffixed-form, -ffree-form and the like, but again this is not necessary directly related to this PR.

Copy link
Member

@gnikit gnikit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, I would suggest you remove the optional argument. Preprocessor directives should just be skipped. Also multi-line preprocessor directives need to be accounted for.

fortls/helper_functions.py Outdated Show resolved Hide resolved
fortls/helper_functions.py Outdated Show resolved Hide resolved
test/test_helper.py Outdated Show resolved Hide resolved
@albertziegenhagel albertziegenhagel force-pushed the fix-form-detect branch 4 times, most recently from 1e5cb0e to 4a14d5e Compare October 19, 2023 11:50
Copy link
Member

@gnikit gnikit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (sorry I completely forgot about this!)

Thanks for the contribution @albertziegenhagel, I will issue a pre-release now.

Currently a file with the content

```
#if defined(A) && !defined(B)
c This is a fixed-form style comment
#endif
```

is categorized as free-form Fortran because the preprocess if-clause contains ampersands and exclamation marks.

This commit makes the detection algorithm ignore all lines starting with `#` if pre-processing is enabled for that file. Pre-processor lines are not valid Fortran code and should IMO not be taken into account when trying to determine the Fortran form.
Additionally, `test_helper.py` has been removed since all the test cases in there are in the examples of the docstring and hence they are covered by doctest already.
@gnikit gnikit merged commit 8466034 into fortran-lang:master Jan 21, 2024
20 checks passed
@albertziegenhagel albertziegenhagel deleted the fix-form-detect branch June 4, 2024 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants