[GHC] #7595: Static flags code needs cleanup
#7595: Static flags code needs cleanup -----------------------------+---------------------------------------------- Reporter: jstolarek | Owner: jstolarek Type: task | Status: new Priority: normal | Component: Driver Version: 7.7 | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Blockedby: Blocking: | Related: -----------------------------+---------------------------------------------- While reading through the source code I noticed that code responsible for handling static flags could use some refactoring: * there are two modules that handle static flags: [[GhcFile(compiler/main/StaticFlags.hs)]] and [[GhcFile(compiler/main/StaticFlagParser.hs)]]. This is about 300 lines of code total and can be easily placed within a single file, especially that division of functions between these two files is sometimes confusing. * [[GhcFile(compiler/main/StaticFlags.hs)]] contains code responsible for handling dynamic flags. I will merge code from [[GhcFile(compiler/main/StaticFlagParser.hs)]] into into [[GhcFile(compiler/main/StaticFlags.hs)]], create a [[GhcFile(compiler/main/StaticFlags.hs-boot)]] to break module dependency cycles and move the code for handling dynamic flags to [[GhcFile(compiler/main/DynFlags.hs)]] -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -----------------------------+---------------------------------------------- Reporter: jstolarek | Owner: jstolarek Type: task | Status: patch Priority: normal | Component: Driver Version: 7.7 | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Blockedby: Blocking: | Related: -----------------------------+---------------------------------------------- Changes (by jstolarek): * status: new => patch Comment: I merged StaticFlags.hs and StaticFlagParser.hs into one file and moved functions that handle dynamic flags into DynFlags.hs -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup
-----------------------------+----------------------------------------------
Reporter: jstolarek | Owner: jstolarek
Type: task | Status: patch
Priority: normal | Component: Driver
Version: 7.7 | Keywords:
Os: Unknown/Multiple | Architecture: Unknown/Multiple
Failure: None/Unknown | Blockedby:
Blocking: | Related:
-----------------------------+----------------------------------------------
Comment(by jan.stolarek@…):
commit a7f9930a24a91cfb5e2579867e5a0b1d83b5a947
{{{
Author: Jan Stolarek
#7595: Static flags code needs cleanup ---------------------------------+------------------------------------------ Reporter: jstolarek | Owner: jstolarek Type: task | Status: closed Priority: normal | Component: Driver Version: 7.7 | Resolution: fixed Keywords: | Os: Unknown/Multiple Architecture: Unknown/Multiple | Failure: None/Unknown Blockedby: | Blocking: Related: | ---------------------------------+------------------------------------------ Changes (by dterei): * status: patch => closed * resolution: => fixed -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: jstolarek Type: task | Status: closed Priority: normal | Milestone: Component: Driver | Version: 7.7 Resolution: fixed | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Changes (by simonmar): * difficulty: => Unknown Comment: 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. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: jstolarek Type: task | Status: closed Priority: normal | Milestone: Component: Driver | Version: 7.7 Resolution: fixed | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Comment(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`. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: high | Milestone: 7.8.1 Component: Driver | Version: 7.7 Resolution: | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Changes (by igloo): * owner: jstolarek => * priority: normal => high * status: closed => new * resolution: fixed => * milestone: => 7.8.1 Comment: 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. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: high | Milestone: 7.8.1 Component: Driver | Version: 7.7 Resolution: | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Comment(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. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: high | Milestone: 7.8.1 Component: Driver | Version: 7.7 Resolution: | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Comment(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. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:8 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: high | Milestone: 7.8.1 Component: Driver | Version: 7.7 Resolution: | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Comment(by simonmar): Replying to [comment:8 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. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: high | Milestone: 7.8.1 Component: Driver | Version: 7.7 Resolution: | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Comment(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. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:10 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: patch Priority: high | Milestone: 7.8.1 Component: Driver | Version: 7.7 Resolution: | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Changes (by jstolarek): * status: new => patch Comment: 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. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: jstolarek Type: task | Status: patch Priority: high | Milestone: 7.8.1 Component: Driver | Version: 7.7 Resolution: | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Changes (by jstolarek): * owner: => jstolarek Comment: Requesting patch review. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:12 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#7595: Static flags code needs cleanup
-------------------------------+--------------------------------------------
Reporter: jstolarek | Owner: jstolarek
Type: task | Status: patch
Priority: high | Milestone: 7.8.1
Component: Driver | Version: 7.7
Resolution: | Keywords:
Os: Unknown/Multiple | Architecture: Unknown/Multiple
Failure: None/Unknown | Difficulty: Unknown
Testcase: | Blockedby:
Blocking: | Related:
-------------------------------+--------------------------------------------
Comment(by jan.stolarek@…):
commit fa8686331d30d7b41f9fc8654e7271819fa14a86
{{{
Author: Jan Stolarek
#7595: Static flags code needs cleanup -------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: jstolarek Type: task | Status: closed Priority: high | Milestone: 7.8.1 Component: Driver | Version: 7.7 Resolution: fixed | Keywords: Os: Unknown/Multiple | Architecture: Unknown/Multiple Failure: None/Unknown | Difficulty: Unknown Testcase: | Blockedby: Blocking: | Related: -------------------------------+-------------------------------------------- Changes (by igloo): * status: patch => closed * resolution: => fixed Comment: I applied the first one. Thanks. -- Ticket URL: http://hackage.haskell.org/trac/ghc/ticket/7595#comment:14 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
participants (1)
-
GHC