MAINT: Decrease the amount of warnings generated by Fluidity

[Patol75]

There is a large number of warnings generated throughout the building and testing phases in Fluidity. It would be nice to try to limit their occurrence. I have currently removed a lot of unused variables, with still more to go. Indeed, there are other kinds of warnings, such as implicit procedures or unused dummy arguments. Any help is appreciated.

[gnikit]

I wholeheartedly agree with this. When we updated to Ubuntu 20 I had to go through a similar process and check all the compiler warnings in case GCC 10 had raised any problematic warnings and it was definitely cumbersome. It would be substantially easier if only "relevant" warnings were displayed.

A bit about the warnings

• The unused functions could be disabled by -Wno-unused-function
• I think the ieee_arithmetic_dummy can be removed completely, which should remove some compiler warnings, and instead we can use and ieee_is_normal, ieee_is_nan, etc. from ieee_arithmetic see here for quick info
• the -Wc-binding-type warnings in Integer_set.F90 and Integer_hash_table.F90 can be removed by changing from the default kind to a C kinds from iso_c_binding/iso_fortran_env
• The -Wunused-dummy-argument are really case dependant and IMO a fair ammount of rewrite would need to be done for them to go away, so for now maybe just turn them off (-Wno-unused-dummy-argument)??
• The -Wconversion warnings e.g. Linked_Lists.F90:663:11 might need us to specify the kinds of integers explicitly, which might be an issue.
• -Wtabs I think can be ignored unless if we don't mind that a couple of files will have big, whitespace git diffs (of course we can always ignore whitespaces in git diffs)
• The -Wmaybe-uninitialized as far as I could tell when we migrated to Ubuntu 20 were all false positives so we can ignore them.
• which brings us to the final (i think) warning -Wimplicit-interface. The solution for this is relatively simple, let us create a couple of modules for BLAS/LAPACK, PETSc, Zoltan and MPI that would contain just the interfaces (MPI already has that). However, when I encountered the same issue with our Radiation Transport code (FETCH) and replaced the barebone BLAS/LAPACK calls with calls from explicitly defined interfaces I noticed a small, but measurable ~3-5% increase in runtime, for medium to large problem sizes. Given that nothing else changed and the results were identical to a relative tolerance of 1e-14 this lead me to believe that explicitly defining an interface in some cases might lead to unnecessary creations of temporary arrays, which in turn degrades performance. Fluidity might not run into this since for every call to BLAS/LAPACK in Fluidity roughly 100-200 could me made in FETCH. I am attaching the BLAS/LAPACK Fortran interfaces if anyone wants to give this a try in Fluidity LAPACK_Interfaces.F90.txt

[stephankramer]

Agreed with most of that. There's a balance here - warnings are not always 100% accurate, and sometimes we ignore them for good practical reasons; switching off all categories that give cases we choose to ignore, may hide valid warnings; but I fully agree that there are way too many at the moment, which means in practice that all warning messages are now being ignored.

Specifically:

• -Wno-unused-function: agreed - although it's nice to remove dead code, this warning doesn't seem to be accurate enough
• removing ieee_arithmetic_dummy: agreed
• changing Integer_set.F90 to bind(c): agreed
• -Wunused-dummy-argument: hm yeah, a little hesistant, but fair enough let's just ignore it then
• -Wconversion: yeah this could be messy, how many of those warnings are there?
• -Wtabs: not sure I agree there, fortran files should not have tabs in them
• -Wmaybe-uninitialized : this one is very useful if it's accurate, but again if it gives too many false positive we'll just end up ignoring them...
• -Wimplicit-interface: this one has always been a headache. I agree that it gives way too many warnings. Unfortunately it's not as easy as just writing our own interfaces for petsc/zoltan/mpi: fortran compilers treat explicit interfaces internally quite differently from implicit interfaces. The extra information you provide, may allow the compiler optimisations that break the internal workings of the library you link with, in particular ones like mpi and petsc that internally use all sorts of c-pointer tricks to interface with what is in its core a c library. Their fortran wrappers do not use the later bind(c) fortran standards to interface with c routines, rather they rely on "in-practice-standards" of how implicit-interfaced fortran routines have been implemented in practice in the compilers that they support. This can get quite tricky: think for instance what happens if you declare an array intent(inout) and the fortran compiler decides use a temp and copy back on return - whereas the routine might actually constitute some asynchronous communication that tries to write into that array after the call has completed. We have tried this in the past and always ended up with very subtle and hard to track memory bugs, and that would vary from version to version of say petsc. The only way we can reliably call these libraries is through their advertised interface (which in the case of petsc is a combination of implicit and explicit interfaces), any subtle issues with new versions of libraries or compilers are usually found earlier by the developers or other users of these libraries - so trying to maintain our own interface is a bit of a fool's errand I think. So on the last point I'm more tempted to also switch that warning off entirely. Any subroutines we write are in modules anyway. The only downside I can see is that if you accidentally call a subroutine that you haven't defined, or you haven't included the necessary use statement for it, you will only get an error at link time.

[Patol75]

Thanks to both of you for the help here.

I have pushed an additional commit that should drastically reduce the number of warnings, that is if it passes CI. Mentioning CI, I have removed Groovy from the YAML, as it is not supported anymore.

Regarding your comments, I have done the following:

• added -Wno-unused-function and -Wno-unused-dummy-argument
• removed -Wimplicit-interface
• removed as many unused variables as I could find
• fixed tabs
• fixed some deprecations in C/C++ code
• fixed a few -Wconversion using the transfer function

Can I ask one of you to deal with ieee_arithmetic_dummy and -Wc-binding-type? I am not fully sure how to proceed for both of them. For the latter, perhaps this is relevant.

Additionally, there is a -Wintrinsic-shadow in C_Interfaces_Fortran.F90:70:37:, if any of you know what it is about.

---

Indeed, I did break things. transfer was not the appropriate thing to do; instead, int is probably the way to go. Also, I tried to play clever with Fortran 77, but I failed miserably, so reverted the changes in libmba2d/makM.f. I forgot to mention there are also a few -Wunknown-pragmas in fldtriangle.cpp and fldgmsh.cpp

---

@gnikit @stephankramer CI is green again, happy for you to commit some further changes according to the above. Also, does any of you know how to remove Groovy from the list of expected checks?

[gnikit]

Awesome work @Patol75, I will have a go at ieee_arithmetic_dummy and -Wc-binding-type. I will also check -Wintrinsic-shadow. Having had a quick look at C_Interfaces_Fortran.F90 I think the module can be removed all together. get_environment_variable is now in the standard, memcpy is not used and compare_pointers can be written in pure fortran.

Let me know if you want me to know if there is anything I can do.

As for the Groovy Actions workflow, I think we run into our branch protection rules see https://github.com/actions/runner/issues/1539 . @stephankramer do you by any chance have access to the Settings of Fluidity or know the name of the new devops person that does (now that Tim is on leave)?

[gnikit]

@stephankramer FYI I had to write a new is_nan that is optimisation-safe because NaNs are optimised with -ffast-math. The previous is_nan worked because it was effectively isnan from C compiled without any optimisations. None of this really matters since all of the is_nan calls in Fluidity are surrounded by asserts, but I thought I should justify this specific change in the codebase.

[Patol75]

@gnikit Thanks a lot for your commits here! I am glad I let you handle ieee_arithmetic_dummy, really great work there (I see you encountered Vladimir F on StackOverflow, the guy knows everything about Fortran!). For -Wc-binding-type, you made me realise my attempt was not successful because I was writing type(c_int) instead of integer(c_int)! So bad haha.

After pulling the new commits (and adding my latest one), make -j now prints about 1500 lines, among which the following types of warnings are still present:

• -Wunused-label -> 3
• -Wconversion -> 97
• Fortran 2018 deleted feature: DO termination statement which is not END DO or CONTINUE with label -> 2
• -Wcatch-value= -> 7
• -Wreturn-type -> 6
• -Wmaybe-uninitialized -> 32
• -Wcharacter-truncation -> 3
• -Winteger-division -> 1
• -Wunused-value -> 2

Additionally, make -j fltools, executed after make -j, now prints about 260 lines, among which the following types of warnings are still present:

• -Wunknown-pragmas -> 4
• -Wconversion -> 38
• -Wunused-value -> 2

Finally, sudo make install and make unittest executed thereafter are warning-free. However, for the latter, there are some problems with tools/gmsh2triangle.py. It seems to me that this script has not been updated to reflect the changes to Gmsh v4. Would you mind having a look @angus-g? Executing make in assemble/tests should yield the error.

Note the maybe_unused attribute that works great for C++ code.

[angus-g]

There are a few GMSH 4 formats, and it's hard to hand-roll scripts that are compatible with all of them. Maybe it would be worth rewriting the tools on top of something like https://github.com/nschloe/meshio, but in the meantime it's probably easier to pass the -format msh2 flag to gmsh in the Makefiles?

[Patol75]

CI is all green, I guess now is a good time to review the changes. Looking back at what I have done, the main wonder I have is if I should have commented out some lines of code instead of deleting them, but I lack experience with Fluidity source code to answer that one.

[angus-g]

I've taken the liberty of rebasing onto latest main, and stripping out the trivial whitespace changes (i.e. EOL removals) while leaving the tab-conversions that alleviate the -Wtabs warnings. It's available at https://github.com/angus-g/fluidity/tree/deal_with_warnings. I agree that it's annoying to have trailing whitespace, but I'm not yet sure if it's worth having giant whitespace diffs just to fix it (yes, there are ways around it, but it's still noisy). I'll defer to @stephankramer for an opinion on that. Otherwise, once this branch is rebased onto main, I'll have a few review suggestions and then we should be good to go!

[Patol75]

Thanks, @angus-g. Regarding whitespaces, I think that the easiest approach is to use pre-commit as mentioned somewhere else by @gnikit. Someone runs it locally, commits all the changes, opens a PR, makes sure CI stays green, and then basically merges. Indeed, pre-commit can then be added to the CI so that all new committed files/changes would be checked. And then we never hear ever again about whitespaces.

[angus-g]

That makes sense, I think that's a reasonable solution. In that case, it would probably be better to do it all at once in its own PR to have a single conflict point (e.g. if there's a merge/rebase spanning that commit), and then add the CI at that point, and have it ready for everything that comes afterwards?

[Patol75]

Indeed, sounds like a good plan. I have rebased as requested. :)

[Patol75]

Thanks, @angus-g.

@stephankramer Can you confirm that this change is fine? Thank you.

[Patol75]

CI returned all green after a re-trigger, let me know if this is good to go.

[stephankramer]

Yes, I'm happy with the changes. It is of course now failing the pre-commit because it doesn't have a pre-commit configuration, meaning that PRs can't be merged without admin override before we merge the pre-commit stuff - which I could do of course, but actually would you mind just merging this branch into the pre-commit PR branch (which I think is ready to go as well once the flake8 stuff is fixed)? Both PRs touch quite a large number of lines in the code, so there's going to be some conflicts, but it's better to resolve these only once, instead of everyone else having to resolve these same conflicts when merging their own life branches.

[Patol75]

All good, closing this PR accordingly.