Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix conversion warnings#536

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
Carltoffel wants to merge2 commits intofortran-lang:master
base:master
Choose a base branch
Loading
fromCarltoffel:fix-conversion-warnings

Conversation

Carltoffel
Copy link
Member

What do you think about these changes?

pro
fixes 12 warnings (the remaining 12 warnings should be easy to fix, too), which are ~ 60 lines tostderr

contra
complexity of code (readability, maintainability, etc.)

Maybe there are better options to fix the warnings, please let me know.

@CarltoffelCarltoffel changed the titleFix conversion warnings in logspaceFix conversion warningsSep 20, 2021
@gareth-nx
Copy link
Contributor

Obviously this is a subjective judgement. My feeling is that, if the original code is legal, then the extra code complexity isn't worth it.

It would be ideal if were compiler options to turn off this kind of warning. But I don't know of any.

milancurcic reacted with thumbs up emoji

@Carltoffel
Copy link
MemberAuthor

It would be ideal if were compiler options to turn off this kind of warning. But I don't know of any.

Usually it is the other way around: You can turn these warningson with compiler options. Build systems tend to activate every warning available, because these warnings are useful (in general).

@gareth-nx
Copy link
Contributor

Yes, I think for this reason gfortran supports turning warnings off, like (-Wno-implicit) as well as on (-Wimplicit). So you can use-Wall and just silence a few of them.

@awvwgk
Copy link
Member

It would be ideal if were compiler options to turn off this kind of warning. But I don't know of any.

We are getting a lot of false-positive and spurious warning from our current CMake setup. See#387 for further details.

@Carltoffel
Copy link
MemberAuthor

We are getting a lot of false-positive and spurious warning from our current CMake setup. See#387 for further details.

Are those conversion warnings I fixed really false-positive? There are implicit conversions happening, so I think these are true-positive warnings. The question is, should we:

  • Make the conversions explicit (verbose & complex code)
  • Hide all conversion warnings (might hide useful warnings)
  • Leave it as is (very verbose output, might also hide useful warnings)

@awvwgk
Copy link
Member

We are using-Wconversion-extra which basically flags every conversion even if it is safe to use.

add_compile_options(-Wconversion-extra)

@Carltoffel
Copy link
MemberAuthor

Okay, so do we want to leave the code as is and simply ignore the warnings?

@awvwgk
Copy link
Member

My suggestion would be to check our compiler warning options first and trim those down to the useful ones. Once we get rid of the 90% false positives, we fix the remaining warnings.

jvdp1 and epagone reacted with thumbs up emoji

@gareth-nx
Copy link
Contributor

I wonder if removing-Wconversion-extra is a good trade-off here?

@Carltoffel
Copy link
MemberAuthor

I did a quick test if we can leave out thekind= arguments, because this would make the code a little more readable. Unfortunately this doesn't solves theConversion from COMPLEX(4) to COMPLEX(16) warnings. But maybe this will be an useful option with a different set of compiler flags.

@jvdp1
Copy link
Member

Most of them are due to specific compiler flags. I would first remove the useless ones, and then check the remaining warnings.

@epagone
Copy link

My suggestion would be to check our compiler warning options first and trim those down to the useful ones. Once we get rid of the 90% false positives, we fix the remaining warnings.

I agree with this. The next step would be to open a bug report with the compiler(s) that should not report false positives obfuscating the useful warnings. I don't think that we should bend stdlib to the needs of the compiler(s).

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@Carltoffel@gareth-nx@awvwgk@jvdp1@epagone

[8]ページ先頭

©2009-2025 Movatter.jp