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

Adds fypp linting support #591

Merged
merged 17 commits into from
Aug 16, 2022
Merged

Adds fypp linting support #591

merged 17 commits into from
Aug 16, 2022

Conversation

gnikit
Copy link
Member

@gnikit gnikit commented Aug 1, 2022

feat: add basic support for fypp

This is the start of incorporating fypp into VS Code

Hi @aradi I have gone ahead and added the simplest solution I could think off for fypp support to get the ball rolling and then,
I thought it would be a good idea to discuss some of the functionality we would want to include and problems I/we will have to address. I will start listing listing some of the things I have already thought off:

TODO

  • Allow for arguments to be passed to fypp
  • Incorporate the existing IncludeDirs passed to linters as --include=${} in fypp
  • Implement an fypp interface to VS Code settings. For the preprocessor defines it makes sense we share them with fortls too. Have 2 settings and merge them in the backend for fortls or have a single option for both? I think the first one is better.
  • Add fypp auto-download with pip
  • Improve the compiler flags passed into the linter when fypp is enabled
  • Add better safety checks for other compilers or add the appropriate compiler flags for fypp to work
  • The linter serves diagnostics on the "expanded" source file we need to find a way to map back to the "compressed" file, i.e. pre-fypp to serve the diagnostics there. I don't know what resources does fypp provide to make that possible.

    I think the solution to this is use --line-numbering, at least for me that works.

  • determine if Fixed or Free form
  • Add unittests
  • Add integration test for fypp
  • Should the preprocessor definitions for fypp and fortls be merged? fortls understands normal Fortran preprocessor only. (I would like to discuss swapping to fypp if it can also handle normal preprocessor directives). deferred to a future conversation.
  • Add support for ifort and nagfor if possible. To be implemented via feat: add fypp linting support for Intel and NAG #615

Fixes #215

@gnikit gnikit self-assigned this Aug 1, 2022
@gnikit gnikit marked this pull request as draft August 1, 2022 17:37
src/features/linter-provider.ts Outdated Show resolved Hide resolved
src/features/linter-provider.ts Outdated Show resolved Hide resolved
src/features/linter-provider.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
gnikit added 3 commits August 2, 2022 13:54
This is the start of incorporating fypp into VS Code
Implements the following:

- Spawn an async process for `fypp` to pass as output to main linter
- Checks for `fypp` in the system and if not installs it
- Adds safety guards to trigger only for `gfortran`
- Uses cached include dirs from the main linter
@gnikit
Copy link
Member Author

gnikit commented Aug 4, 2022

@aradi is fypp capable of handling normal Fortran preprocessor commands? I know that mixing normal preprocessor directive and fypp ones is not a good idea, so I am not talking about this case.

From my little local testing all should be good, but the normal preprocessor directives going into fortls and the fypp directives going into fypp should be kept separate.

Another topic that you might be interested in discussing is substituting or adding fypp support to fortls. Currently the normal preprocessor parser I have written is garbage and I am actively looking for better alternatives (in an attempt to avoid having to write my own). If fypp is able to somehow integrate with fortls providing both a normal and an fypp aware preprocessor that would be great.

@aradi
Copy link
Member

aradi commented Aug 4, 2022

No, fypp can't handle cpp-grammar. Actually, fypp does not have a proper lexer/tokenizer, it only looks for simple patterns via regular expressions. Most of the content is then passed to Python, which will parse then the expressions using its own parser. If you want to implement cpp-grammar, things get more complicated, as you would have to write a proper lexer and expression parser, so I think, it won't really profit from anything within fypp. I know that PreForM can deal with a bunch of the cpp-constructs, but not sure, whether that is of any help for you.

@awvwgk
Copy link
Member

awvwgk commented Aug 4, 2022

For a Python based cpp you can always use https://github.com/ned14/pcpp

@gnikit
Copy link
Member Author

gnikit commented Aug 11, 2022

For a Python based cpp you can always use https://github.com/ned14/pcpp

Cheers @awvwgk pcpp has been on my radar for quite some time now, but I recall having some problems with it. Tbh, it might be worth just writing a C preproc from scratch.

gnikit added 4 commits August 11, 2022 15:30
Addresses review comments
The linter and `fypp` include paths are quite different and should
be set using different options.

Direct comment from PR:
"""
The include paths for fypp (where fypp include files should be looked for) are typically pointing to folders in a projects source tree. While the -I options, you pass to a compiler in order to look up mod-files, are typically pointing to folders in the projects build tree. So, strictly speaking, there are completely different and should be treated differently. On the other hand, passing -I options meant for the compiler to fypp won't make any harm, as long as they do not contain source files included via #:include in any fypp-processed files. Chances for that are pretty low.
"""
@gnikit gnikit mentioned this pull request Aug 12, 2022
@gnikit
Copy link
Member Author

gnikit commented Aug 14, 2022

@aradi I think at this stage I will add fypp support only for GCC. My thought process behind this is that for ifort, nagfor (and any other compiler) we would have to create the actual fypp expanded files. I have found the correct way of doing that, see #614 but doing so in this PR would cause a lot of changes. More importantly, if we go down that path I am not sure having the on-the-fly (no temp files) fypp --> gfortran functionality would make a lot of sense.

@gnikit gnikit marked this pull request as ready for review August 15, 2022 13:01
@gnikit gnikit requested review from aradi and awvwgk August 15, 2022 13:01
@gnikit
Copy link
Member Author

gnikit commented Aug 15, 2022

This is now done, any additional work will be implemented in a separate PR, @aradi @awvwgk have a look if you want. I will me merging this later today.

@gnikit gnikit merged commit cd218ef into main Aug 16, 2022
@gnikit gnikit deleted the feature/fypp branch August 16, 2022 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Support for fypp preprocessor
3 participants