Commented up some potential issues with GOOL

code/drasil-gool/lib/Drasil/GOOL/InterfaceGOOL.hs

@@ -134,6 +134,7 @@ class (ValueSym r, OOTypeSym r) => OOValueSym r
 class (VariableSym r, OOTypeSym r) => OOVariableSym r where
   -- Bool: False for variable, True for constant.  Required by the Python renderer.
   staticVar'    :: Bool -> Label -> VSType r -> SVariable r
+  -- We should probably have a way to make this a constant that can't have an L-Value
   self         :: SVariable r
   classVar     :: VSType r -> SVariable r -> SVariable r
   extClassVar  :: VSType r -> SVariable r -> SVariable r

code/drasil-gool/lib/Drasil/GOOL/RendererClassesOO.hs

@@ -40,10 +40,12 @@ class (BlockCommentSym r) => RenderFile r where
 
   fileFromData :: FilePath -> IG.FSModule r -> IG.SFile r
 
+-- This assumes a certain representation for Permanence
 class PermElim r where
   perm :: r (IG.Permanence r) -> Doc
   binding :: r (IG.Permanence r) -> Binding
 
+-- Why do we have these here on top of InterfaceCommon?
 class InternalGetSet r where
   getFunc :: SVariable r -> VSFunction r
   setFunc :: VSType r -> SVariable r -> SValue r -> VSFunction r
@@ -63,6 +65,7 @@ class (RenderMethod r, OOMethodTypeSym r) => OORenderMethod r where
 
   destructor :: [IG.CSStateVar r] -> SMethod r
 
+-- This assumes a certain representation for StateVar
 class StateVarElim r where  
   stateVar :: r (IG.StateVar r) -> Doc
 
@@ -85,5 +88,6 @@ class RenderMod r where
   modFromData :: String -> FS Doc -> IG.FSModule r
   updateModuleDoc :: (Doc -> Doc) -> r (IG.Module r) -> r (IG.Module r)
 
+-- This assumes a certain representation for Module
 class ModuleElim r where
   module' :: r (IG.Module r) -> Doc

code/drasil-gool/lib/Drasil/Shared/InterfaceCommon.hs

@@ -89,6 +89,8 @@ class TypeSym r where
   funcType      :: [VSType r] -> VSType r -> VSType r
   void          :: VSType r
 
+-- We've established that getTypeString is a bad thing
+-- getType might be required, but without global state should it be a class?
 class (TypeSym r) => TypeElim r where
   getType :: r (Type r) -> CodeType
   getTypeString :: r (Type r) -> String
@@ -108,6 +110,7 @@ class (TypeSym r) => VariableSym r where
   extVar    :: Library -> Label -> VSType r -> SVariable r
   arrayElem :: Integer -> SVariable r -> SVariable r
 
+-- Similar here - shouldn't we be handling this on a per-language basis?
 class (VariableSym r) => VariableElim r where
   variableName :: r (Variable r) -> String
   variableType :: r (Variable r) -> r (Type r)
@@ -120,6 +123,7 @@ listOf = listVar
 
 type SValue a = VS (a (Value a))
 
+-- Similar here - is this the best way to get the type of a value?
 class (TypeSym r) => ValueSym r where
   type Value r
   valueType :: r (Value r) -> r (Type r)
@@ -256,6 +260,8 @@ libFuncApp l n t vs = libFuncAppMixedArgs l n t vs []
 exists :: (ValueExpression r) => SValue r -> SValue r
 exists = notNull
 
+-- These all return SValue, but shouldn't they return MSStatement?
+-- Also, they have the issue of not distinguishing Values and L-Values (See below)
 class (ValueSym r) => List r where
   -- | Does any necessary conversions from GOOL's zero-indexed assumptions to
   --   the target language's assumptions
@@ -282,6 +288,7 @@ class (ValueSym r) => List r where
   --   Arguments are: List, Value
   indexOf :: SValue r -> SValue r -> SValue r
 
+-- Similar to List, shouldn't most of these be statements?
 class (ValueSym r) => Set r where
   -- | Checks membership
   -- Arguments are: Set, Value
@@ -377,6 +384,8 @@ assignToListIndex :: (StatementSym r, VariableValue r, List r) => SVariable r
   -> SValue r -> SValue r -> MSStatement r
 assignToListIndex lst index v = valStmt $ listSet (valueOf lst) index v
 
+-- As Reed mentioned, these have a lack of distinction between values/variables
+-- and L-Values
 class (VariableSym r, StatementSym r, ScopeSym r) => DeclStatement r where
   varDec       :: SVariable r -> r (Scope r) -> MSStatement r
   varDecDef    :: SVariable r -> r (Scope r) -> SValue r -> MSStatement r
@@ -392,7 +401,7 @@ class (VariableSym r, StatementSym r, ScopeSym r) => DeclStatement r where
   funcDecDef   :: SVariable r -> r (Scope r) -> [SVariable r] -> MSBody r
     -> MSStatement r
 
-
+-- More places that could benefit from L-Values
 class (VariableSym r, StatementSym r) => IOStatement r where
   print      :: SValue r -> MSStatement r
   printLn    :: SValue r -> MSStatement r

code/drasil-gool/lib/Drasil/Shared/LanguageRenderer/LanguagePolymorphic.hs

@@ -206,6 +206,7 @@ objVar o' v' = do
         (variableType v) (R.objVar (RC.variable o) (RC.variable v))
   objVar' (variableBind v)
 
+-- This seems a bit sketchy - unwrapping everything to Doc, mixing it up, then wrapping it back to Variable
 arrayElem :: (OORenderSym r) => SValue r -> SVariable r -> SVariable r
 arrayElem i' v' = do
   i <- IC.intToIndex i'

code/drasil-gool/lib/Drasil/Shared/RendererClassesCommon.hs

@@ -50,6 +50,7 @@ class (AssignStatement r, DeclStatement r, IOStatement r,
 
 -- Common Typeclasses --
 
+-- Couldn't this just be in InterfaceCommon?
 class ImportSym r where
   type Import r
   -- For importing an external library
@@ -69,18 +70,23 @@ class BodyElim r where
 class RenderBlock r where
   multiBlock :: [MSBlock r] -> MSBlock r
 
+-- This assumes that the renderers encode Body as Doc
 class BlockElim r where
   block :: r (Block r) -> Doc
 
+-- This relates to TypeData, which keeps ADT, String, and Doc representations of a type for _reasons_
 class RenderType r where
   multiType :: [VSType r] -> VSType r
   typeFromData :: CodeType -> String -> Doc -> VSType r
 
+-- This is coupled to TypeData
 class InternalTypeElim r where
   type' :: r (Type r) -> Doc
 
 type VSUnOp a = VS (a (UnaryOp a))
 
+-- Why do we have these, when we also have NumericExpression in InterfaceCommon?
+-- It looks like the implementations for NumericExpression link here, but why?
 class UnaryOpSym r where
   type UnaryOp r
   notOp    :: VSUnOp r
@@ -124,12 +130,16 @@ class OpElim r where
   uOpPrec :: r (UnaryOp r) -> Int
   bOpPrec :: r (BinaryOp r) -> Int
 
+-- These assume we represent Scope with ScopeData
 class ScopeElim r where
   scopeData :: r (Scope r) -> ScopeData
-
+
+-- What exactly is this, since it adds a bunch of extra required data?
 class RenderVariable r where
   varFromData :: Binding -> String -> VSType r -> Doc -> SVariable r
-
+
+-- This assumes variables are represented in a certain way, and
+-- its existence seems a bit sketchy
 class InternalVarElim r where
   variableBind :: r (Variable r) -> Binding
   variable  :: r (Variable r) -> Doc
@@ -157,6 +167,7 @@ class ValueElim r where
   valueInt :: r (Value r) -> Maybe Integer
   value :: r (Value r) -> Doc
 
+-- Again, wh are we recreating all these list functions?
 class InternalListFunc r where
   -- | List
   listSizeFunc   :: SValue r -> VSFunction r
@@ -172,10 +183,12 @@ class InternalListFunc r where
 class RenderFunction r where
   funcFromData :: Doc -> VSType r -> VSFunction r
 
+-- Assumes a certain representation for functions
 class FunctionElim r where
   functionType :: r (Function r) -> r (Type r)
   function :: r (Function r) -> Doc
 
+-- Shouldn't this be in InterfaceCommon?
 class InternalAssignStmt r where
   multiAssign       :: [SVariable r] -> [SValue r] -> MSStatement r 
 
@@ -198,13 +211,16 @@ class StatementElim r where
 
 class RenderVisibility r where
   visibilityFromData :: VisibilityTag -> Doc -> r (Visibility r)
-
+
+-- This assumes a certain representation of visibility 
 class VisibilityElim r where
   visibility :: r (Visibility r) -> Doc
 
+-- Why do we need a Doc here?
 class RenderParam r where
   paramFromData :: SVariable r -> Doc -> MSParameter r
 
+-- This assumes a certain representation for Parameter
 class ParamElim r where
   parameterName :: r (Parameter r) -> Label
   parameterType :: r (Parameter r) -> r (Type r)
@@ -216,6 +232,7 @@ class BlockCommentSym r where
   -- | Converts a list of strings into a block comment
   docComment :: State a [String] -> State a (r (BlockComment r))
 
+-- This assumes a certain representation for BlockComment
 class BlockCommentElim r where
   blockComment' :: r (BlockComment r) -> Doc
 
@@ -230,5 +247,6 @@ class (MethodTypeSym r, BlockCommentSym r) => RenderMethod r where
   commentedFunc :: MS (r (BlockComment r)) -> SMethod r -> SMethod r
   mthdFromData :: VisibilityTag -> Doc -> SMethod r
 
+-- This assumes a certain representation for Method
 class MethodElim r where
   method :: r (Method r) -> Doc