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

Draft: initial implementation of implicit_none #506

Closed
wants to merge 4 commits into from

Conversation

certik
Copy link
Member

@certik certik commented Jun 29, 2021

@epagone and I worked on this and implemented the following.

This adds implicit_none flag to fpm.toml, if it is not specified, it will be .false. by default.

Then it is propagated to the model, at the top level (for now). Then in the backend we read this flag from the model and insert -fimplicit-none into the command line flag (so it only works for gfortran for now).

TODO:

  • It should be integrated with the rest of the compiler options / profiles (currently we add it in fpm_backend.f90 which is probably too late, it should be already part of target%compile_flags)
  • Generate the correct flag for each compiler
  • Add a test for this
  • This should be set on per package basis and even allow to override per file basis
  • Design question: should this flag by part of the model as is done in this PR (perhaps on per package/per file basis) or should it be collected directly from fpm.toml and put into compiler flags / profiles and never expose it explicitly in the model?
  • Turn this on by default

@certik certik requested a review from awvwgk June 29, 2021 17:05
Comment on lines +269 to +273
if (model%implicit_none) then
implicit_flag = " -fimplicit-none"
else
implicit_flag = ""
end if
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like the right place to add this option. At this point we are not aware which compiler we are using anymore and cannot be sure that we are actually passing the correct flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see the first TODO item.

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Let's not add this option to the top-level in the manifest, but use the build sub-table instead. The top-level currently only contains meta data which does not affect the handling of the package, while all settings which change the behaviour of the build are currently found in the build sub-table.

How about implicit-typing as name for the key? For consistency with other options, I would also prefer a dash rather than an underscore for multiword keys.

@certik
Copy link
Member Author

certik commented Jun 29, 2021

Yes, implicit-typing=false should be the default then in the long run.

Yes, we couldn't quickly figure out how to do it on the per-package basis instead of global. We have to look into the build subtable.

@certik
Copy link
Member Author

certik commented Jul 13, 2021

@epagone will you have time to finish this PR?

@epagone
Copy link
Contributor

epagone commented Jul 13, 2021

Yes, I do not intend to let it go stale, sorry for the waiting. This week is extremely busy, I plan on having a look at it next week.

@certik
Copy link
Member Author

certik commented Jul 13, 2021

Awesome, thanks! If you get stuck, please let me know, I can help.

@awvwgk awvwgk added the specification Issue regarding fpm manifest and model label Aug 4, 2021
@certik
Copy link
Member Author

certik commented Aug 10, 2021

@epagone just a friendly ping. :)

@MarDiehl
Copy link

The corresponding option for ifort is -warn declaration

@everythingfunctional
Copy link
Member

I would suggest implementing this on top of the profiles feature. The logic is already worked out for propagating profiles, this would introduce logic about how different parts of the profiles are propagated.

@awvwgk
Copy link
Member

awvwgk commented Aug 12, 2021

I agree with Brad that we should build this on the compiler profiles branch instead.

@certik
Copy link
Member Author

certik commented Aug 13, 2021

Can somebody more familiar with fpm internals create a prototype how to hook this up with an option in fpm toml? I can then try to polish it.

@awvwgk
Copy link
Member

awvwgk commented Aug 13, 2021

Sure, but we need to handle some prerequisites first. We need some abstraction for the compiler to actually retrieve the flag (#451 is a start) and we need a more refined model to adjust flags on per project basis (#498).

@awvwgk
Copy link
Member

awvwgk commented Sep 23, 2021

I can take a look into implementing this feature once we merged #575, which implements the necessary infrastructure to make this possible. But since this is also a straight-forward addition and some of you already started working on it, I'll also offer to only give a few pointers on how this is could be implemented and than hold myself back. Let me know what you preference here is.

In any case, this PR is not a good place to discuss this feature. Therefore, I suggest we close this PR since it has significantly diverged from the main branch and continue the discussion at #577.

@epagone
Copy link
Contributor

epagone commented Sep 23, 2021

Yes, I would like to take a shot at this, please. I had exactly the same plan in mind: wait for the work on the flags to stabilise a minimum and then ask for some pointers. I can't promise that I will be fast but, if you agree, I want to give it a try.

@awvwgk awvwgk closed this Sep 23, 2021
@certik
Copy link
Member Author

certik commented Sep 23, 2021

Sure, that's fine. As long as it gets done. :)

@epagone
Copy link
Contributor

epagone commented Sep 24, 2021

As long as it gets done. :)

If that is your order, yessir! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
specification Issue regarding fpm manifest and model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants