[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 <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(-) }}} -- Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/7595#comment:2> 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 | 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 <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(-) }}} -- Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/7595#comment:13> 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: 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