On 13 February 2012 04:53, Simon Marlow <[email protected]> wrote:
> On 10/02/2012 19:38, David Terei wrote:
>
>> -checkAdd :: Bool -> String -> GHCi InteractiveImport
>> +-- TODO: ARGH! This is a mess! 'checkAdd' is called from many places and
>> we
>> +-- have about 4 different variants of setGHCContext. All this import code
>> needs
>> +-- to be refactored to something saner. We should do the sanity check on
>> an
>> +-- import in 'checkAdd' and checkAdd only and only need to call checkAdd
>> from
>> +-- one place ('setGHCContetFromGHCiState'). The code isn't even logically
>> +-- ordered!
>
>
> FWIW I did refactor this code recently, for about the 4th time, and it's
> pretty carefully put together. I'm sorry it didn't make sense to you, it
> probably needs some more comments.
>
> Can you explain what it is you want to achieve?
>
> Cheers,
> Simon
Originally you'd get this behaviour:
Prelude> :set -XSafe
Prelude> import System.IO.Unsafe
<no location info>:
base:System.IO.Unsafe can't be safely imported! The module itself
isn't safe.
Prelude> import Data.Char
<no location info>:
base:System.IO.Unsafe can't be safely imported! The module itself
isn't safe.
Prelude Data.Char>
My understanding of all this code is:
* ghci maintains its own import context as GHC will drop the context
under some situations so ghci needs to do this for persistence
* checkAdd makes sure a module is OK to import (e.g exists, is byte
code form if required, [and now] is safe if required). At least that
seems the purpose of checkAdd to me but maybe I'm wrong as I find the
code structure strange.
* setGHCContext... functions take the context ghci maintains and loads
it into ghc.
* sometimes we want to load a bunch of imports as a batch operation.
My problem with the code is all the entry points. It feels as if the
code has just grown more complex for each case required and is quite
spaghetti like now.
e.g the 'setContext' function takes two lists of imports, maps
'checkAdd' over them and saves the new context. It then calls
'setGHCContextFromGHCiState' which does basically (map over the input
with checkAdd) the same thing but calls into GHC at the end to set the
actual context.
What I want is one interface to this all. So a setContext method that
handles calling 'checkAdd' and 'addNotSubsumed' and can handle batch
or individual additions. Then for all the :module, :import commands I
just call this 'setContext'. Based on how I've seen the code used this
seems quite doable.
Cheers,
David.
>
>
>
>
>
>> +checkAdd :: Bool -> String -> GHCi (InteractiveImport)
>> checkAdd star mstr = do
>> dflags<- getDynFlags
>> case safeLanguageOn dflags of
>> @@ -1664,8 +1661,8 @@ checkAdd star mstr = do
>> s<- GHC.isModuleTrusted m
>> case s of
>> True -> return $ IIDecl (simpleImportDecl $ moduleName
>> m)
>> - False -> ghcError $ CmdLineError $ "can't import " ++
>> mstr
>> - ++ " as it isn't
>> trusted."
>> + False -> ghcError $ CmdLineError $
>> + "can't import " ++ mstr ++ " as it isn't trusted."
>>
>> False | star -> do m<- wantInterpretedModule mstr
>> return $ IIModule m
>> @@ -1687,12 +1684,26 @@ checkAdd star mstr = do
>> --
>> setGHCContextFromGHCiState :: GHCi ()
>> setGHCContextFromGHCiState = do
>> - let ok (IIModule m) = checkAdd True (moduleNameString (moduleName m))
>> - ok (IIDecl d) = checkAdd False (moduleNameString (unLoc
>> (ideclName d)))
>> st<- getGHCiState
>> - iidecls<- filterM (tryBool . ok) (transient_ctx st ++ remembered_ctx
>> st)
>> - setGHCContext iidecls
>> + goodTran<- filterM (tryBool . ok) $ transient_ctx st
>> + goodRemb<- filterM (tryBool . ok) $ remembered_ctx st
>> + -- drop bad imports so we don't keep replaying it to the user!
>> + modifyGHCiState $ \s -> s { transient_ctx = goodTran }
>> + modifyGHCiState $ \s -> s { remembered_ctx = goodRemb }
>> + setGHCContext (goodTran ++ goodRemb)
>>
>> + where
>> + ok (IIModule m) = checkAdd True (moduleNameString (moduleName m))
>> + ok (IIDecl d) = checkAdd False (moduleNameString (unLoc (ideclName
>> d)))
>> +
>> +setContext :: [String] -> [String] -> GHCi ()
>> +setContext starred not_starred = do
>> + is1<- mapM (checkAdd True) starred
>> + is2<- mapM (checkAdd False) not_starred
>> + let iss = foldr addNotSubsumed [] (is1++is2)
>> + modifyGHCiState $ \st -> st { remembered_ctx = iss, transient_ctx = []
>> }
>> + -- delete the transient context
>> + setGHCContextFromGHCiState
>>
>> -- | Sets the GHC contexts to the given set of imports, adding a Prelude
>> -- import if there isn't an explicit one already.
>> @@ -2745,7 +2756,7 @@ tryBool :: GHCi a -> GHCi Bool
>> tryBool m = do
>> r<- ghciTry m
>> case r of
>> - Left _ -> return False
>> + Left e -> showException e>> return False
>> Right _ -> return True
>>
>> --
>> ----------------------------------------------------------------------------
>>
>>
>>
>> _______________________________________________
>> Cvs-ghc mailing list
>> [email protected]
>> http://www.haskell.org/mailman/listinfo/cvs-ghc
>
>
_______________________________________________
Cvs-ghc mailing list
[email protected]
http://www.haskell.org/mailman/listinfo/cvs-ghc
