Use C99's bool
ClosedPublic

Authored by bgamari on Nov 12 2016, 9:12 AM.

Details

Summary

Just as it says on the tin.

Test Plan

Validate on lots of platforms

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.
bgamari updated this revision to Diff 9382.Nov 12 2016, 9:12 AM
bgamari retitled this revision from to Fix type of GarbageCollect declaration.
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
bgamari updated this revision to Diff 9383.Nov 12 2016, 9:13 AM
bgamari edited edge metadata.

Upload everything

bgamari retitled this revision from Fix type of GarbageCollect declaration to Use C99's bool.Nov 12 2016, 9:16 AM
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
bgamari edited edge metadata.
bgamari updated this revision to Diff 9384.Nov 12 2016, 9:16 AM

One more commit

michalt added inline comments.
includes/rts/Flags.h
189–194

Maybe change indent to be consistent with the other fields?

rts/Interpreter.c
1620

Maybe simplify to just interruptible?

rts/Profiling.c
686–691

Maybe simplify to just return the condition?

697–702

ditto

rts/RtsDllMain.c
33–39

This is unrelated to your changes, but maybe remove the switch (potentially leaving the comment)?

rts/STM.c
120–126

Seems like this could also be simplified.

946

Maybe remove == true?

rts/linker/Elf.c
62–63

Those should be unnecessary (or am I missing something?)

bgamari marked 6 inline comments as done.Nov 12 2016, 4:02 PM

Thanks for the review, @michalt!

rts/linker/Elf.c
62–63

Nope, you are right.

bgamari updated this revision to Diff 9388.Nov 12 2016, 4:03 PM
bgamari marked an inline comment as done.
bgamari edited edge metadata.
  • michalt's comments
erikd accepted this revision.Nov 12 2016, 4:31 PM
erikd edited edge metadata.
This revision is now accepted and ready to land.Nov 12 2016, 4:31 PM

Thanks for the review, @michalt!

Hope it was useful. :-) Thanks for a nice cleanup!

bgamari updated this revision to Diff 9393.Nov 13 2016, 9:52 AM
bgamari edited edge metadata.

Fix STM

simonmar edited edge metadata.Nov 14 2016, 3:22 AM

This is great! I never liked rtsBool :)

I haven't looked through all of it, but there are a couple of concerns inline.

compiler/codeGen/StgCmmBind.hs
286 ↗(On Diff #9393)

overzealous search/replace?

compiler/coreSyn/CoreUtils.hs
2111–2120 ↗(On Diff #9393)

here too

rts/win32/AsyncIO.c
247

This is a Win32 API and probably doesn't use bool.

rts/win32/ConsoleHandler.c
46–47

Is this really legit? This is a Win32 API and probably doesn't use bool, so we might be getting implicit promotion.

bgamari updated this revision to Diff 9409.Nov 14 2016, 4:51 PM
bgamari marked 5 inline comments as done.
bgamari edited edge metadata.
  • Simon's comments
simonmar added inline comments.Nov 15 2016, 4:22 AM
rts/sm/GC.h
20–21

What's going on here? I don't see any changes to the calls.

rts/win32/OSThreads.c
141

More Win32 APIs

562

here too

bgamari marked 2 inline comments as done.Nov 15 2016, 11:11 AM
bgamari added inline comments.
rts/STM.c
946

I think I'll leave this one; if nothing else it's a reminder that cas returns a bool.

rts/sm/GC.h
20–21

Yes, this is actually a separate commit locally since it wasn't entirely obvious what was going on. The definition of GarbageCollect in GC.c disagreed with this declaration. This change brings the two into agreement. I checked the call-sites and they should all still be correct.

rts/win32/AsyncIO.c
247

Fair point. I think they are semantically equivalent, but perhaps we should play it safe.

rts/win32/OSThreads.c
141

Bah. Thanks for catching these.

bgamari updated this revision to Diff 9431.Nov 15 2016, 11:20 AM
bgamari marked an inline comment as done.
bgamari edited edge metadata.
  • More Win32 silliness
simonmar accepted this revision.Nov 16 2016, 7:23 AM
simonmar edited edge metadata.

You have build breakage, but otherwise looks fine to me.

rts/sm/GC.h
20–21

Oh... rtsBool is the same as uint32_t, so there was no warning.

bgamari updated this revision to Diff 9499.Nov 18 2016, 10:24 AM
bgamari edited edge metadata.
  • Bulk replacement
  • rts: Use stdbool
  • ghc/hschooks.c: Fix include path of Rts.h
  • Fix type of GarbageCollect declaration
  • michalt's comments
  • Simon's comments
  • More Win32 silliness
  • rts: Fix bell
bgamari added inline comments.Nov 18 2016, 10:26 AM
rts/Stats.c
246

@simonmar, this was the cause of the build issues. As far as I can tell this was dead code previously as bell could never be > 1 (at least assuming RtsFlags were set from the command line).

bgamari updated this revision to Diff 9518.Nov 20 2016, 11:57 AM
bgamari edited edge metadata.
  • rts: Fix bell
bgamari updated this revision to Diff 9524.Nov 21 2016, 11:23 AM
bgamari edited edge metadata.
  • rts: Ensure that RtsFlags is initialized
  • Initialize ringBell
bgamari updated this revision to Diff 9533.Nov 21 2016, 3:51 PM
bgamari edited edge metadata.
  • Never mind

Strangely enough I'm seeing the RtsFlags.GcFlags.ringBell get set from defaultsHook in the stage1 compiler. This results in validation failure due to unexpected ^G output on stderr. I'm a bit baffled by this, although have a sneaking suspicion that the problem lies in deriveConstants, which uses the wrong width for bool. However, exactly how this happens isn't clear on reading the code.

bgamari updated this revision to Diff 9659.Nov 29 2016, 12:25 PM
bgamari edited edge metadata.
  • Bulk replacement
  • rts: Use stdbool
  • michalt's comments
  • Simon's comments
  • More Win32 silliness
  • rts: Fix bell
  • Initialize ringBell
This revision was automatically updated to reflect the committed changes.