Enable new warning for fragile/incorrect CPP #if usage
ClosedPublic

Authored by erikd on Mar 4 2017, 6:11 PM.

Details

Summary

The C code in the RTS now gets built with -Wundef and the Haskell code
(stages 1 and 2 only) with -Wcpp-undef. We now get warnings whereever
#if is used on undefined identifiers.

Test Plan

Validate on Linux and Windows

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
erikd created this revision.Mar 4 2017, 6:11 PM

Looks good to me. I'm however a bit irritated about the inconsistent use of

  • #ifdef
  • #if defined X
  • #if defined(X).

Do we have any general rule when to use one or the other?

erikd added a comment.Mar 4 2017, 7:49 PM

@angerman If that was the only inconsistency in the GHC code base it would be well worth the effort to make it consistent. Furthermore, I think having coding requirements that are not backed up by either the compiler or linting tools is a waste of time. Happy to defer to others if their opinions differ from mine.

Phyx accepted this revision.Mar 5 2017, 5:35 AM
This revision is now accepted and ready to land.Mar 5 2017, 5:35 AM
erikd updated this revision to Diff 11583.Mar 6 2017, 1:50 AM

Fix some compile errors

I don't think I ever use #if defined X, and I prefer #if defined(X) over #ifdef. But if we're moving towards having configuration symbols be always defined, it doesn't really matter. I agree with @erikd that whatever style is agreed on should be automatically linted.

bgamari requested changes to this revision.Mar 6 2017, 5:58 PM

I believe this breaks T8103.

This revision now requires changes to proceed.Mar 6 2017, 5:58 PM
bgamari added inline comments.Mar 6 2017, 6:01 PM
includes/CodeGen.Platform.hs
991

Let's try to be more consistent here. I tend to prefer #if defined(BLAH) but either way works for me.

erikd updated this revision to Diff 11754.Mar 15 2017, 3:25 AM
  • Rebase against master
  • Fix minor formatting issue @bgamari noticed.

Test T8103 still failing and can't (yet) figure out why.

bgamari requested changes to this revision.Mar 21 2017, 12:51 PM

Requesting changes on account of testsuite failures.

This revision now requires changes to proceed.Mar 21 2017, 12:51 PM
erikd added a comment.EditedMar 21 2017, 3:28 PM

I've tracked down which file is causing that test failure, but still haven't figured out why.

erikd added a comment.Mar 26 2017, 4:45 AM

If I revert the changes to includes/stg/MachRegs.h and the changes to rts/ghc.mk, mk/wranings.mk (to disable the warnings) it builds fine and passes the T8103 test.

Phyx requested changes to this revision.Mar 26 2017, 5:37 AM

Looking at these again, I suspect a lot of these aren't right. If you look at includes/stg/HaskellMachRegs.h it seems like these MACHREGS_ macros are always defined. just to either 0 or 1.
By switching these from #if to #if defined(..) they're now all trivially always true.

erikd updated this revision to Diff 11891.Mar 27 2017, 5:57 AM
  • Fix T8103 test.
  • Rebase against master.
erikd updated this revision to Diff 11897.Mar 27 2017, 1:50 PM

Improve consistency.

erikd updated this revision to Diff 11898.Mar 28 2017, 4:29 AM
  • Rebase against master.
  • Fix OS X build.
erikd updated this revision to Diff 11910.EditedMar 29 2017, 2:21 AM
  • Rebase against master.
  • Fix another OSX build issue.
erikd updated this revision to Diff 11911.Mar 29 2017, 5:04 AM
  • More OSX fixes.
erikd updated this revision to Diff 11941.Mar 31 2017, 4:58 PM

Yet more OS X fixes.

erikd updated this revision to Diff 11951.Apr 1 2017, 5:40 AM

Yet more OS X fixes.

erikd added a comment.Apr 1 2017, 8:22 PM

This finally seems to be ready to go.

bgamari accepted this revision.Apr 3 2017, 9:12 AM

Yay! Great work @erikd.

erikd updated this revision to Diff 11986.Apr 3 2017, 9:19 PM

Rebase against master

This revision was automatically updated to reflect the committed changes.