Enable more warnings (but add also exceptions)#8
Merged
Conversation
Collaborator
|
A good summary with good points.
|
Member
Author
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. |
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
As discussed,
-Wno-compare-realsis 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
selfdummy 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 sowhich could then be used to "use"
selfasnop(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-shadowis required to allow for the redefinition ofCOUNT. 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 forb3hs_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-procedurefortestAppendingListsonly... maybe this can also be easily fixed?