Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions copilot-c99/src/Copilot/Compile/C99/CodeGen.hs
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,14 @@ mkStructDecln (Struct x) = C.TypeDecln struct

mkField :: Value a -> C.FieldDecln
mkField (Value ty field) = C.FieldDecln (transType ty) (fieldName field)
mkStructDecln _ = error "Unhandled case"

-- | Write a forward struct declaration.
mkStructForwDecln :: Struct a => Type a -> C.Decln
mkStructForwDecln (Struct x) = C.TypeDecln struct
where
struct = C.TypeSpec $ C.Struct (typeName x)
mkStructForwDecln _ = error "Unhandled case"

-- * Ring buffers

Expand Down
18 changes: 10 additions & 8 deletions copilot-c99/src/Copilot/Compile/C99/Expr.hs
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ transExpr (Op2 (UpdateField ty1@(Struct _) ty2 f) e1 e2) = do
e2' <- transExpr e2

-- Variable to hold the updated struct
(i, _, _) <- get
let varName = "_v" ++ show i
(i', _, _) <- get
let varName = "_v" ++ show i'
Comment on lines +64 to +65
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

modify (\(i, x, y) -> (i + 1, x, y))

-- Add new var decl
Expand Down Expand Up @@ -101,14 +101,14 @@ transExpr (Op2 op e1 e2) = do
e2' <- transExpr e2
return $ transOp2 op e1' e2'

transExpr e@(Op3 (UpdateArray arrTy@(Array ty2)) e1 e2 e3) = do
transExpr (Op3 (UpdateArray arrTy@(Array ty2)) e1 e2 e3) = do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

e1' <- transExpr e1
e2' <- transExpr e2
e3' <- transExpr e3

-- Variable to hold the updated array
(i, _, _) <- get
let varName = "_v" ++ show i
(i', _, _) <- get
let varName = "_v" ++ show i'
Comment on lines +110 to +111
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

modify (\(i, x, y) -> (i + 1, x, y))

-- Add new var decl
Expand All @@ -117,18 +117,19 @@ transExpr e@(Op3 (UpdateArray arrTy@(Array ty2)) e1 e2 e3) = do
modify (\(i, x, y) -> (i, x ++ [initDecl], y))

let size :: Type (Array n t) -> C.Expr
size arrTy@(Array ty) = C.LitInt (fromIntegral $ typeLength arrTy)
size arrTy'@(Array ty) = C.LitInt (fromIntegral $ typeLength arrTy')
C..* C.SizeOfType (C.TypeName $ transType ty)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The * is aligned with the =. Can you align it to match?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size _ = error "Unhandled case"

-- Initialize the var to the same value as the original array
let initStmt = C.Expr $ memcpy (C.Ident varName) e1' (size arrTy)

-- Update element of array
let updateStmt = case ty2 of
Array _ -> C.Expr $ memcpy dest e3' size
Array _ -> C.Expr $ memcpy dest e3' size'
where
dest = C.Index (C.Ident varName) e2'
size = C.LitInt
size' = C.LitInt
(fromIntegral $ typeSize ty2)
C..* C.SizeOfType (C.TypeName (tyElemName ty2))
Comment on lines 131 to 134
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make this change, we'd have to align the surrounding lines accordingly. The = in the dest definition and the two lines under size would both have to be indented by one more space.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Expand Down Expand Up @@ -183,6 +184,7 @@ transOp1 op e =
BwNot _ -> (C..~) e
Cast _ ty -> C.Cast (transTypeName ty) e
GetField (Struct _) _ f -> C.Dot e (accessorName f)
_ -> error "Unhandled case"

-- | Translates a Copilot binary operator and its arguments into a C99
-- expression.
Expand Down
2 changes: 1 addition & 1 deletion copilot-c99/src/Copilot/Compile/C99/Representation.hs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ data UniqueTrigger = UniqueTrigger UniqueTriggerId Trigger

-- | Given a list of triggers, make their names unique.
mkUniqueTriggers :: [Trigger] -> [UniqueTrigger]
mkUniqueTriggers ts = zipWith mkUnique ts [0..]
mkUniqueTriggers ts = zipWith mkUnique ts [0 :: Integer ..]
where
mkUnique t@(Trigger name _ _) n = UniqueTrigger (name ++ "_" ++ show n) t
24 changes: 11 additions & 13 deletions copilot-c99/tests/Test/Copilot/Compile/C99.hs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ import Test.QuickCheck.Gen (chooseAny, chooseBoundedIntegral)

-- External imports: Copilot
import Copilot.Core hiding (Property)
import Copilot.Core.Type.Array (array)

-- External imports: Modules being tested
import Copilot.Compile.C99 (cSettingsOutputDirectory, compile, compileWith,
mkDefaultCSettings)
Expand Down Expand Up @@ -434,15 +432,15 @@ funCompose2 (f1, g1) (f2, g2) (f3, g3) =

-- | Test case specification for specs with one input variable and one output.
data TestCase1 a b = TestCase1
{ wrapTC1Expr :: Spec
Spec
-- ^ Specification containing a trigger an extern of type 'a' and a trigger
-- with an argument of type 'b'.

, wrapTC1Fun :: [a] -> [b]
([a] -> [b])
-- ^ Function expected to function in the same way as the Spec being
-- tested.

, wrapTC1CopInp :: (String -> String, String, String, Gen a)
(String -> String, String, String, Gen a)
-- ^ Input specification.
--
-- - The first element is a function that prints the variable declaration
Expand All @@ -456,26 +454,26 @@ data TestCase1 a b = TestCase1
-- - The latter contains a randomized generator for values of the given
-- type.

, wrapTC1CopOut :: (String, String)
(String, String)
-- ^ Output specification.
--
-- The first element of the tuple contains the type of the output in C.
--
-- The second element of the tuple is the formatting string when printing
-- values of the given kind.
}


-- | Test case specification for specs with two input variables and one output.
data TestCase2 a b c = TestCase2
{ wrapTC2Expr :: Spec
Spec
-- ^ Specification containing a trigger an extern of type 'a' and a trigger
-- with an argument of type 'b'.

, wrapTC2Fun :: [a] -> [b] -> [c]
([a] -> [b] -> [c])
-- ^ Function expected to function in the same way as the Spec being
-- tested.

, wrapTC2CopInp1 :: (String -> String, String, String, Gen a)
(String -> String, String, String, Gen a)
-- ^ Input specification for the first input.
--
-- - The first element is a function that prints the variable declaration
Expand All @@ -489,7 +487,7 @@ data TestCase2 a b c = TestCase2
-- - The latter contains a randomized generator for values of the given
-- type.

, wrapTC2CopInp2 :: (String -> String, String, String, Gen b)
(String -> String, String, String, Gen b)
-- ^ Input specification for the second input.
--
-- - The first element is a function that prints the variable declaration
Expand All @@ -503,14 +501,14 @@ data TestCase2 a b c = TestCase2
-- - The latter contains a randomized generator for values of the given
-- type.

, wrapTC2CopOut :: (String, String)
(String, String)
-- ^ Output specification.
--
-- The first element of the tuple contains the type of the output in C.
--
-- The second element of the tuple is the formatting string when printing
-- values of the given kind.
}


-- | Generate test cases for expressions that behave like unary functions.
mkTestCase1 :: (Typed a, Typed b)
Expand Down
1 change: 0 additions & 1 deletion copilot-core/copilot-core.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ library

ghc-options:
-Wall
-fno-warn-orphans

Comment on lines 41 to 42
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this. Why is it needed? Is it because it's being added to the specific modules that need it?

Copy link
Author

@yaitskov yaitskov Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the warning tuning flag is used once (Copilot/Core/Type.hs)

build-depends:
base >= 4.9 && < 5
Expand Down
9 changes: 7 additions & 2 deletions copilot-core/src/Copilot/Core/Type.hs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{-# OPTIONS_GHC -fno-warn-orphans #-}
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE ExistentialQuantification #-}
{-# LANGUAGE FlexibleContexts #-}
Expand Down Expand Up @@ -48,6 +49,7 @@ module Copilot.Core.Type
import Data.Char (isLower, isUpper, toLower)
import Data.Coerce (coerce)
import Data.Int (Int16, Int32, Int64, Int8)
import qualified Data.Kind as DK
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make this change, we have to align the surrounding imports (the module names need to be aligned, and the symbols imported need to be aligned. I normally use stylish-haskell for that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import Data.List (intercalate)
import Data.Proxy (Proxy (..))
import Data.Type.Equality as DE
Expand Down Expand Up @@ -139,7 +141,7 @@ instance {-# OVERLAPPABLE #-} (Typed t, Struct t) => Show t where
-- Note that both arrays and structs use dependently typed features. In the
-- former, the length of the array is part of the type. In the latter, the
-- names of the fields are part of the type.
data Type :: * -> * where
data Type :: DK.Type -> DK.Type where
Bool :: Type Bool
Int8 :: Type Int8
Int16 :: Type Int16
Expand All @@ -165,6 +167,7 @@ typeLength _ = fromIntegral $ natVal (Proxy :: Proxy n)
typeSize :: forall n t . KnownNat n => Type (Array n t) -> Int
typeSize ty@(Array ty'@(Array _)) = typeLength ty * typeSize ty'
typeSize ty@(Array _ ) = typeLength ty
typeSize ty = error $ "There is a bug in the type checker " ++ show ty

instance TestEquality Type where
testEquality Bool Bool = Just DE.Refl
Expand Down Expand Up @@ -286,7 +289,9 @@ instance Typed Double where

instance (Typeable t, Typed t, KnownNat n) => Typed (Array n t) where
typeOf = Array typeOf
simpleType (Array t) = SArray t
simpleType t = case t of
Array t' -> SArray t'
o -> error $ "There is a bug in the type checker " ++ show o

-- | A untyped type (no phantom type).
data UType = forall a . Typeable a => UType (Type a)
Expand Down
4 changes: 2 additions & 2 deletions copilot-core/src/Copilot/Core/Type/Array.hs
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ arrayUpdate (Array []) _ _ = error errMsg
where
errMsg = "copilot-core: arrayUpdate: Attempt to update empty array"

arrayUpdate (Array (x:xs)) 0 y = Array (y:xs)
arrayUpdate (Array (_x:xs)) 0 y = Array (y:xs)
Copy link
Member

@ivanperez-keera ivanperez-keera Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just make it _?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


arrayUpdate (Array (x:xs)) n y =
arrayAppend x (arrayUpdate (Array xs) (n - 1) y)
where
-- | Append to an array while preserving length information at the type
-- level.
arrayAppend :: a -> Array (n - 1) a -> Array n a
arrayAppend x (Array xs) = Array (x:xs)
arrayAppend x' (Array xs') = Array (x':xs')
4 changes: 1 addition & 3 deletions copilot-core/tests/Test/Copilot/Core/Type.hs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ module Test.Copilot.Core.Type where

-- External imports
import Data.Int (Int16, Int32, Int64, Int8)
import Data.Maybe (isJust)
import Data.Proxy (Proxy (..))
import Data.Type.Equality (TestEquality (..), testEquality,
(:~:) (..))
Expand Down Expand Up @@ -136,8 +135,7 @@ testSimpleTypesEqualityTransitive =
-- | Test that each type is only equal to itself.
testSimpleTypesEqualityUniqueness :: Property
testSimpleTypesEqualityUniqueness =
forAllBlind (shuffle simpleTypes) $ \(t:ts) ->
notElem t ts
forAllBlind (shuffle simpleTypes) $ \x -> case x of [] -> False; (t:ts) -> notElem t ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line goes over 80 characters. Can we wrap after -> or of?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


-- | Simple types tested.
simpleTypes :: [SimpleType]
Expand Down
8 changes: 4 additions & 4 deletions copilot-core/tests/Test/Copilot/Core/Type/Array.hs
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,13 @@ testArrayUpdateWrong len =
testArrayMakeWrongLength :: forall n . KnownNat n => Proxy n -> Property
testArrayMakeWrongLength len =
expectFailure $
forAll wrongLength $ \length ->
forAll (xsInt64 length) $ \ls ->
forAll wrongLength $ \length' ->
forAll (xsInt64 length') $ \ls ->
let array' :: Array n Int64
array' = array ls
in arrayElems array' == ls
where
xsInt64 length = vectorOf length arbitrary
xsInt64 length' = vectorOf length' arbitrary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we align the $? Also, an alternative, shorter name could be len.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expectedLength = fromIntegral (natVal len)
wrongLength = (expectedLength +) . getNonNegative <$> arbitrary

Expand All @@ -157,7 +157,7 @@ testArrayUpdateElems :: forall n . KnownNat n => Proxy n -> Property
testArrayUpdateElems len =
forAll xsInt64 $ \ls ->
forAll position $ \p ->
forAll xInt64 $ \v ->
forAll xInt64 $ \_v ->
let -- Original array
array' :: Array n Int64
array' = array ls
Expand Down
28 changes: 15 additions & 13 deletions copilot-interpreter/src/Copilot/Interpret/Eval.hs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module Copilot.Interpret.Eval
import Copilot.Core (Expr (..), Field (..), Id, Name, Observer (..),
Op1 (..), Op2 (..), Op3 (..), Spec, Stream (..),
Trigger (..), Type (..), UExpr (..), Value (..),
arrayElems, arrayUpdate, specObservers,
arrayElems, arrayUpdate, specObservers,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is just a stylistic change that doesn't otherwise fix a warning, I'd like to keep that separate. We try to get PRs to focus on one concern only (can be over multiple files and commits, but still just focused on solving one problem).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tab replaced with spaces

specStreams, specTriggers, updateField)
import Copilot.Interpret.Error (badUsage)

Expand All @@ -32,7 +32,6 @@ import Data.Dynamic (Dynamic, fromDynamic, toDyn)
import Data.List (transpose)
import Data.Maybe (fromJust)
import Data.Typeable (Typeable)
import GHC.TypeLits (KnownNat, Nat, natVal)

-- | Exceptions that may be thrown during interpretation of a Copilot
-- specification.
Expand Down Expand Up @@ -142,14 +141,15 @@ type LocalEnv = [(Name, Dynamic)]
evalExpr_ :: Typeable a => Int -> Expr a -> LocalEnv -> Env Id -> a
evalExpr_ k e0 locs strms = case e0 of
Const _ x -> x
Drop t i id ->
let Just buff = lookup id strms >>= fromDynamic in
reverse buff !! (fromIntegral i + k)
Local t1 _ name e1 e2 ->
Drop _t i id ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the type is not captured in the Const case, we can probably just use _.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case lookup id strms >>= fromDynamic of
Just buff -> reverse buff !! (fromIntegral i + k)
Nothing -> error $ "No id " ++ show id ++ " in " ++ show strms
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we align the ->?

Also, is this a suitable way to show a message to a user who is not a regular Haskeller? Should the message be presented differently?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess Nothing branch is unreachable code

Local _t1 _ name e1 e2 ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't capture the type in the Const case, we can probably just use _.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let x = evalExpr_ k e1 locs strms in
let locs' = (name, toDyn x) : locs in
x `seq` locs' `seq` evalExpr_ k e2 locs' strms
Var t name -> fromJust $ lookup name locs >>= fromDynamic
Var _t name -> fromJust $ lookup name locs >>= fromDynamic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't capture the type in the Const case, we can probably just use _.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we'd want to keep the -> aligned (in this and in all cases).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExternVar _ name xs -> evalExternVar k name xs
Op1 op e1 ->
let ev1 = evalExpr_ k e1 locs strms in
Expand Down Expand Up @@ -210,6 +210,7 @@ evalOp1 op = case op of
BwNot _ -> complement
Cast _ _ -> P.fromIntegral
GetField (Struct _) _ f -> unfield . f
GetField {} -> error "There is a bug in the type checker"
where
-- Used to help GHC pick a return type for ceiling/floor
idI :: Integer -> Integer
Expand Down Expand Up @@ -247,11 +248,12 @@ evalOp2 op = case op of
BwShiftR _ _ -> ( \ !a !b -> shiftR a $! fromIntegral b )
Index _ -> \xs n -> (arrayElems xs) !! (fromIntegral n)

UpdateField (Struct _) ty (fieldAccessor :: a -> Field s b) ->
UpdateField (Struct _) ty (_fieldAccessor :: a -> Field s b) ->
\stream fieldValue ->
let newField :: Field s b
newField = Field fieldValue
in updateField stream (Value ty newField)
UpdateField {} -> error "There is a bug in the type checker"

-- | Apply a function to two numbers, so long as the second one is
-- not zero.
Expand All @@ -267,14 +269,14 @@ catchZero f x y = f x y
-- 'Copilot.Core.Operators.Op3'.
evalOp3 :: Op3 a b c d -> (a -> b -> c -> d)
evalOp3 (Mux _) = \ !v !x !y -> if v then x else y
evalOp3 (UpdateArray ty) = \xs n x -> arrayUpdate xs (fromIntegral n) x
evalOp3 (UpdateArray _ty) = \xs n x -> arrayUpdate xs (fromIntegral n) x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't capture the type in the Mux case, we can probably just use _.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In either case, we'd need to align the =.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


-- | Turn a stream into a key-value pair that can be added to an 'Env' for
-- simulation.
initStrm :: Stream -> (Id, Dynamic)
initStrm Stream { streamId = id
, streamBuffer = buffer
, streamExprType = t } =
} =
Copy link
Member

@ivanperez-keera ivanperez-keera Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd have to remove the extra space in the two preceding lines, before the =, while still keeping them aligned. Also, if this fits in one line under 80 characters now, we can do that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(id, toDyn (reverse buffer))

-- | Evaluate several streams for a number of steps, producing the environment
Expand All @@ -293,7 +295,7 @@ evalStreams top specStrms initStrms =
strms_ = map evalStream specStrms
evalStream Stream { streamId = id
, streamExpr = e
, streamExprType = t } =
} =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd have to remove the extra space in the two preceding lines, before the =, while still keeping them aligned. Also, if this fits in one line under 80 characters now, we can do that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let xs = fromJust $ lookup id strms >>= fromDynamic in
let x = evalExpr_ k e [] strms in
let ls = x `seq` (x:xs) in
Expand Down Expand Up @@ -402,5 +404,5 @@ showWit t =
Word64 -> ShowWit
Float -> ShowWit
Double -> ShowWit
Array t -> ShowWit
Struct t -> ShowWit
Array _ -> ShowWit
Struct _ -> ShowWit
Comment on lines +403 to +404
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Loading