Skip to content

Conversation

@sloede
Copy link
Member

@sloede sloede commented Jun 8, 2021

I enabled a few more warnings that weren't causing any troubles right now. I also had to disable some that were causing errors, and added the reason as a comment:

FFLAGS += -Wno-compare-reals # Must be allowed since it is used *properly* in several places        
FFLAGS += -Wno-unused-dummy-argument # Must be allowed for some OOP style functions                 
FFLAGS += -Wno-intrinsic-shadow # Must be allowed for COUNT                                         
FFLAGS += -Wno-implicit-interface # Must be allowed for b3hs_hash_key_jenkins

As discussed, -Wno-compare-reals is not easy to remove, since it is heavily relied upon. Thus, I don't think there's anything we can do about it.

There are some methods that accept a self dummy argument but don't use it, which requires the use of -Wno-unused-dummy-argument. A possible way around this I found on Stackoverflow, using a "no operation" dummy definition like so

#define nop(x) associate( x => x ); end associate

which could then be used to "use" self as nop(self). But it seems very hacky and thus I'd just leave it as it is - also, unused arguments typically do not cause bugs in the implementation, but only make the implementation more "unclean", which is not a big deal I think.

-Wno-intrinsic-shadow is required to allow for the redefinition of COUNT. Since you already mentioned that this is part of FTOL's API, I guess this should stay as well.

Thus, the only one that I'd consider fixing is -Wno-implicit-interface, which is - as far as I can tell - only required for b3hs_hash_key_jenkins. However, I am not experienced enough in Fortran to know if an easy & clean fix is available for this. Do you think we can and should fix this, @DavidAKopriva? Otherwise, we can just leave it as it is, I have no strong feelings here.

EDIT: I also had to add -Wno-implicit-procedure for testAppendingLists only... maybe this can also be easily fixed?

@DavidAKopriva
Copy link
Collaborator

A good summary with good points.

  1. There is a set of FP comparisons in HOHQMesh that can be eliminated, and I am doing that now on a local branch. But we will have to keep the others in here unless someone comes up with a "safe" way to do floating point comparisons without having to compare to 0.0 at some point. It is also part of the API for FTValue that will convert an FP number to logical depending on if the argument is 0 or not zero. (I guess that could be fixed with a cast to integer.)
  2. I have used the no-op kind of trick in a few places. (e.g. IF(self % refCount() >= 0) CONTINUE )
    But since the code has been written over the course of a decade, features have been added, but not necessarily retroactively. The whole issue is a quirk of the fortran language which requires explicit passing of the this/self object even if it is not used.
  3. I used count because that's what is used elsewhere (other languages/frameworks) for counting the number of objects in a collection, and is shorter than "numberOfObjects", but it is easy to find and could be changed. I'll look into that.
  4. It is interesting that implicit procedures are now considered something to warn about, since has been an integral part of the language since the 1950's. Depending on dependency ordering, the fix is pretty easy simply by putting those routines in a module. The advantage of implicit interfaces is lack of dependencies, making compilation easier. The disadvantage is there is no checks on arguments. The makefiles would have to be modified for the new dependencies. HOHQMesh has a lot of files with implicit interfaces.

@sloede
Copy link
Member Author

sloede commented Jun 8, 2021

A good summary with [...] with implicit interfaces.

Thank you for this elaborate explanation. I propose to merge this PR as it is right now, and then reference our discussion in a new issue. I don't think "fixing" these issues (if they are issue that need fixing at all) is a high priority right now, but I think it would be nice to have the other extra warnings (plus the use of standard Fortran without GNU extensions) enabled before changing more code.

@DavidAKopriva
Copy link
Collaborator

Yes, do that merge now, since everything compiles fine with the current flags. We can address those other features over time, and my comments hopefully will serve as a reminder as to what needs to be done to get rid of the special cases.

@sloede sloede merged commit 25efdf5 into master Jun 8, 2021
@sloede sloede deleted the more-compiler-flags branch June 8, 2021 18:39
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