Opened 3 years ago

Closed 2 years ago

#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 Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

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 3 years ago.
t7595-static-flags-code-cleanup-fix-nocse-pragma.patch (762 bytes) - added by jstolarek 2 years ago.
Adds -fno-cse pragma to DynFlags.hs
t7595-static-flags-code-cleanup-revert.patch (20.8 KB) - added by jstolarek 2 years ago.
Reverts the original patch

Download all attachments as: .zip

Change History (17)

Changed 3 years ago by jstolarek

comment:1 Changed 3 years 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 3 years ago by jan.stolarek@…

commit a7f9930a24a91cfb5e2579867e5a0b1d83b5a947

Author: Jan Stolarek <[email protected]>
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 <[email protected]>

 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 3 years ago by dterei

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

comment:4 Changed 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 2 years ago by jstolarek

Adds -fno-cse pragma to DynFlags.hs

Changed 2 years ago by jstolarek

Reverts the original patch

comment:11 Changed 2 years 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 2 years ago by jstolarek

  • Owner set to jstolarek

Requesting patch review.

comment:13 Changed 2 years ago by jan.stolarek@…

commit fa8686331d30d7b41f9fc8654e7271819fa14a86

Author: Jan Stolarek <[email protected]>
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 2 years 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.