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

Fix warnings [-Wunused-xxx] from compilation #879

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cyrilgandon
Copy link

@cyrilgandon cyrilgandon commented Oct 2, 2024

Cleaning duty, warnings are generates with

fpm build --verbose --flag '-Wunused-variable -Wunused-dummy-argument -Wunused-parameter'

Progress

  • 42 unused-variable
  • 15 unused-parameter
  • 12/67 unused-dummy-argument
  • 55/67 unused-dummy-argument

Leaving 55 unused-dummy-argument warnings unresolvable, see conversation about those.

@cyrilgandon cyrilgandon changed the title Remove warning -Wunused-variable from compilation Fix warningq -Wunused-xxx from compilation Oct 2, 2024
@cyrilgandon cyrilgandon changed the title Fix warningq -Wunused-xxx from compilation Fix warnings [-Wunused-xxx] from compilation Oct 2, 2024
Copy link
Contributor

@perazz perazz left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @cyrilgandon.

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.

Thanks for sharing

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Thank you @cyrilgandon for this PR. I have only one minor comment regarding Python procedures. Recently, a procedure of Python >3.9 (?) was introduced in the Fortran stdlib, limiting its compilation on systems supporting most recent versions of Python. It would be nice if this issue can be avoided, hence my question below.

src/stdlib_linalg_least_squares.fypp Show resolved Hide resolved
Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Based on @perazz 's answer, this PR can be merged IMO. Thank you @cyrilgandon for cleaning the code.

@cyrilgandon
Copy link
Author

I still have some fix to do on unused-dummy-argument. This is a bit more complicated because I have to really understand what the code does for some cases

@@ -95,6 +95,7 @@ module stdlib_str2num
integer(int8) :: p !! position within the number
integer(int8) :: stat !! error status
!----------------------------------------------
if (.false.) v = mold ! removes unused-dummy-argument warning
Copy link
Member

Choose a reason for hiding this comment

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

What does the compiler with this line? Does it remove it? What about the efficiency of adding this line of the whole procedure?

Choose a reason for hiding this comment

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

I would refrain from such changes. In OO programming, variables defined in abstract routines of the base type can easily be unused in some inherited types. Adding some lines just to suppress the warning is artificial and error-prone. I would rather advocate an option in fpm to build without the argument -Wunused-XXX

Copy link
Contributor

Choose a reason for hiding this comment

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

Fully second leaving the unused dummy arguments as they are. In this simple case the compiler will remove the line, but it is indeed error prone and artificial. Unless one is 100% sure that the procedures signature can be safely changed, then the best solution in this case is to don't do anything.

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.

6 participants