#7595 closed task (fixed)

Static flags code needs cleanup

Reported by: jstolarek Owned by: jstolarek
Priority: high Milestone: 7.8.1
Component: Driver Version: 7.7
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

Description

While reading through the source code I noticed that code responsible for handling static flags could use some refactoring:

I will merge code from compiler/main/StaticFlagParser.hs into into compiler/main/StaticFlags.hs, create a compiler/main/StaticFlags.hs-boot to break module dependency cycles and move the code for handling dynamic flags to compiler/main/DynFlags.hs

Attachments (3)

t7595-static-flags-code-cleanup.patch (21.0 KB) - added by jstolarek 18 months ago.
t7595-static-flags-code-cleanup-fix-nocse-pragma.patch (762 bytes) - added by jstolarek 18 months ago.
Adds -fno-cse pragma to DynFlags.hs
t7595-static-flags-code-cleanup-revert.patch (20.8 KB) - added by jstolarek 18 months ago.
Reverts the original patch

Download all attachments as: .zip

Change History (17)

Changed 18 months ago by jstolarek

comment:1 Changed 18 months ago by jstolarek

  • Status changed from new to patch

I merged StaticFlags.hs and StaticFlagParser.hs into one file and moved functions that handle dynamic flags into DynFlags.hs

comment:2 Changed 18 months ago by jan.stolarek@…

commit a7f9930a24a91cfb5e2579867e5a0b1d83b5a947

Author: Jan Stolarek <jan.stolarek@p.lodz.pl>
Date:   Wed Jan 16 14:21:07 2013 +0100

    StaticFlags code cleanup (fixes #7595)
    
    Function responsible for parsing the static flags, that were spread
    across two modules (StaticFlags and StaticFlagParser), are now
    in one file. This is analogous to dynamic flags parsing, which is
    also contained within a single module.
    
    Signed-off-by: David Terei <davidterei@gmail.com>

 compiler/ghc.cabal.in             |    1 -
 compiler/main/DynFlags.hs         |   22 +++-
 compiler/main/DynFlags.hs-boot    |    8 +-
 compiler/main/GHC.hs              |    5 +-
 compiler/main/StaticFlagParser.hs |  151 -----------------------
 compiler/main/StaticFlags.hs      |  241 ++++++++++++++++++++++++++-----------
 compiler/main/StaticFlags.hs-boot |    4 +
 compiler/utils/Outputable.lhs     |    5 +-
 ghc/Main.hs                       |    1 -
 9 files changed, 204 insertions(+), 234 deletions(-)

comment:3 Changed 18 months ago by dterei

  • Resolution set to fixed
  • Status changed from patch to closed

comment:4 Changed 18 months ago by simonmar

  • Difficulty set to Unknown

I was a bit late here, but as a general point we like to avoid adding new .hs-boot files unless it's unavoidable or there's a lot of benefit to be had. In this case I'd say it's not a clear win. The patch is in now so I wouldn't back it out, but something to bear in mind for the future.

comment:5 Changed 18 months ago by jstolarek

I thought that since DynFlags.hs has a .hs-boot file it will be fine to create one for StaticFlags.hs. If this is really an issue it is always possible to undo the patch with git revert.

comment:6 Changed 18 months ago by igloo

  • Milestone set to 7.8.1
  • Owner jstolarek deleted
  • Priority changed from normal to high
  • Resolution fixed deleted
  • Status changed from closed to new

I'd agree re avoiding .hs-boot files where possible.

Also, DynFlags now contains a GLOBAL_VAR, but doesn't have a {-# OPTIONS -fno-cse #-} pragma. I'm not sure what effect such a pragma would have on the other code in the module, if any.

comment:7 Changed 18 months ago by igloo

Merging the two static flags modules sounds good to me, incidentally. I can't remember whether that necessarily means having a .hs-boot, though.

comment:8 follow-up: Changed 18 months ago by jstolarek

Without .hs-boot there are cyclic module dependencies. Perhaps it would be possible to move around a couple of functions from one module to another and remove the cycle in this way but I fear this would cause more harm than good.

I'll look into the -fno-cse pragma tomorrow.

comment:9 in reply to: ↑ 8 Changed 18 months ago by simonmar

Replying to jstolarek:

Without .hs-boot there are cyclic module dependencies. Perhaps it would be possible to move around a couple of functions from one module to another and remove the cycle in this way but I fear this would cause more harm than good.

Right, the idea is to structure the code so that there is no cycle. That's why occasionally you'll see things in a strange-looking place, it's to avoid a cycle. Cycles (with .hs-boot files) mean less optimisation, and overall a more complex and hard-to-modify codebase.

comment:10 Changed 18 months ago by jstolarek

I see. I didn't know that. Perhaps Wiki should have a page for the beginners with list of things not to do :)

I think that in terms of code readability this patch is a win but if it really prevents optimisations or makes code harder to maintain it can be reverted.

Changed 18 months ago by jstolarek

Adds -fno-cse pragma to DynFlags.hs

Changed 18 months ago by jstolarek

Reverts the original patch

comment:11 Changed 18 months ago by jstolarek

  • Status changed from new to patch

I'm submitting two patches. One adds -fno-cse pragma to DynFlags.hs. The code validates on my machine, but I'm unable to tell if the pragma has any influence on other functions in DynFlags.hs

Alternatively, if introducing new .hs-boot file is a Bad Thing as Simon says, the second patch can be used to revert the changes made by my original patch.

comment:12 Changed 17 months ago by jstolarek

  • Owner set to jstolarek

Requesting patch review.

comment:13 Changed 17 months ago by jan.stolarek@…

commit fa8686331d30d7b41f9fc8654e7271819fa14a86

Author: Jan Stolarek <jan.stolarek@p.lodz.pl>
Date:   Wed Jan 30 07:59:57 2013 +0100

    Add -fno-cse pragma in DynFlags.hs (fixes #7595)

 compiler/main/DynFlags.hs |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

comment:14 Changed 17 months ago by igloo

  • Resolution set to fixed
  • Status changed from patch to closed

I applied the first one. Thanks.

Note: See TracTickets for help on using tickets.