Enable new warning for fragile/incorrect CPP #if usage
Needs ReviewPublic

Authored by erikd on Sat, Mar 4, 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

erikd created this revision.Sat, Mar 4, 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.Sat, Mar 4, 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.Sun, Mar 5, 5:35 AM
This revision is now accepted and ready to land.Sun, Mar 5, 5:35 AM
erikd updated this revision to Diff 11583.Mon, Mar 6, 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.Mon, Mar 6, 5:58 PM

I believe this breaks T8103.

This revision now requires changes to proceed.Mon, Mar 6, 5:58 PM
bgamari added inline comments.Mon, Mar 6, 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.Wed, Mar 15, 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.Tue, Mar 21, 12:51 PM

Requesting changes on account of testsuite failures.

This revision now requires changes to proceed.Tue, Mar 21, 12:51 PM
erikd added a comment.EditedTue, Mar 21, 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.Sun, Mar 26, 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.Sun, Mar 26, 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.Mon, Mar 27, 5:57 AM
  • Fix T8103 test.
  • Rebase against master.
erikd updated this revision to Diff 11897.Mon, Mar 27, 1:50 PM

Improve consistency.

erikd updated this revision to Diff 11898.Tue, Mar 28, 4:29 AM
  • Rebase against master.
  • Fix OS X build.