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

style: treat warnings as errors #26

Merged
merged 8 commits into from
Sep 29, 2024

Conversation

vil02
Copy link
Member

@vil02 vil02 commented Sep 24, 2024

Continuation of #18.

@vil02 vil02 force-pushed the warnings_as_errors branch from e302350 to adf9901 Compare September 24, 2024 15:44
@vil02 vil02 mentioned this pull request Sep 24, 2024
@vil02 vil02 marked this pull request as ready for review September 24, 2024 15:47
@Ramy-Badr-Ahmed
Copy link
Member

Continuation of #18.

Thanks @vil02 ,

Now my local version has become in sync 🙂

I set it up like like this:
add_compile_options(-Wall -Wextra -Wpedantic -Wunused-variable -Werror)

Interested about other complile options?

@vil02
Copy link
Member Author

vil02 commented Sep 24, 2024

I set it up like like this: add_compile_options(-Wall -Wextra -Wpedantic -Wunused-variable -Werror)

-Wunused-variable seams to be part of -Wall.

Interested about other complile options?

Sure, but I would be careful here: some of the warnings are already included into the used flags, some of the warnings are Fortran version specific etc.

@Ramy-Badr-Ahmed
Copy link
Member

-Wunused-variable seams to be part of -Wall.

Right, I'll change my local gfortran version.

Sure, but I would be careful here: some of the warnings are already included into the used flags, some of the warnings are Fortran version specific etc.

I previously used these:

-Wconversion-extra     # Warn about implicit conversions that may cause issues
-fno-backtrace         # Enable backtrace information for runtime errors
-fcheck=all            # Enable all runtime checks (bounds, uninitialized, etc.)
-finit-real=nan        # Initialize all REAL variables to NaN.
-finit-integer=0       # Initializes all integer variables to 0.
-ffpe-trap=all         # Trap floating-point exceptions (zero,overflow,underflow)

@vil02
Copy link
Member Author

vil02 commented Sep 27, 2024

@Ramy-Badr-Ahmed: I added -Wconversion-extra in fb742a5.

Regarding other flags, which you listed: they are not warnings and the discussion about them is out of scope of this PR. Few remarks for the future:

  • I would not rely on -finit-real and -finit-integer but rather initialize all of the variable in the code,
  • might make sense to use -fcheck=all and -ffpe-trap=all in the CI,
  • -fno-backtrace is good for local debugging, in my opinion,
  • we might want to introduce two types of builds: release and debug.

@Ramy-Badr-Ahmed
Copy link
Member

Thanks for the feedback! I agree with your points, especially regarding introducing release and debug builds.

@Ramy-Badr-Ahmed: I added -Wconversion-extra in fb742a5.

Regarding other flags, which you listed: they are not warnings and the discussion about them is out of scope of this PR. Few remarks for the future:

Ok 👍

I would not rely on -finit-real and -finit-integer but rather initialize all of the variable in the code,

Yes, that's good practice. However, in the context of CI, these flags can help identify potential errors for contributors.

For example:

real :: x
x = x + 1.0  ! x is uninitialized

If compiled with -finit-real=nan, the program will crash or return NaN when it encounters x = NaN + 1.0.

might make sense to use -fcheck=all and -ffpe-trap=all in the CI,

-fno-backtrace is good for local debugging, in my opinion,

we might want to introduce two types of builds: release and debug.

For the builds, we could structure it like this:

  • Debug build:

    • Include flags like -fcheck=all, -ffpe-trap=all, and keep -finit-real=nan, -finit-integer for uninitialized variable detection.
    • Keep backtrace enabled for detailed error reporting.
  • Release build:

    • Strip runtime checks and use optimizations like -O2/-O3 for better performance.

@vil02
Copy link
Member Author

vil02 commented Sep 27, 2024

I do not have enough knowledge about FORTRAN to judge relevance of the other warning flags.

@Ramy-Badr-Ahmed: all of this sounds like a good idea for the future work.
@SatinWukerORIG: please consider this PR as ready for a review. I will not push anymore changes here unless necessary.

Copy link
Member

@SatinWukerORIG SatinWukerORIG left a comment

Choose a reason for hiding this comment

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

This is a good way to eliminate potential conflicts and improve code quality for learners to understand. I am merging it 👍

@SatinWukerORIG SatinWukerORIG merged commit ef4b15c into TheAlgorithms:main Sep 29, 2024
3 checks passed
@vil02 vil02 deleted the warnings_as_errors branch September 29, 2024 17:25
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.

3 participants