tuura / plato

A DSL for asynchronous circuits specification
Other
12 stars 2 forks source link

Improved `handleArcs` function. #51

Closed jrbeaumont closed 7 years ago

jrbeaumont commented 7 years ago

Removed explicit recursion.

Added extra to the cabal file, required for groupSortOn.

jrbeaumont commented 7 years ago

Thanks for the comments, I will get to this ASAP. It should be possible to do some work on most of these.

This will break when there are more than 26 signals: show (Signal i) = [chr (ord 'A' + i)]. Think how to improve.

I agree this needs improvement, but I don't believe it can be a quick fix, and it can be fixed with #35, allowing a user to define their own signal names.

snowleopard commented 7 years ago

I agree this needs improvement, but I don't believe it can be a quick fix

Well, there are plenty of quick fixes! For example:

show (Signal i)
    | i < 26    = [chr (ord 'A' + i)]
    | otherwise = 'S' : show i

and it can be fixed with #35, allowing a user to define their own signal names.

True, that would be preferable.

jrbeaumont commented 7 years ago
instance Show Signal where show (Signal i)
    | i < 26    = [chr (ord 'A' + i)]
    | otherwise = 'S' : show i

This throws errors, it doesn't like the |. I'm gonna come back to this soon. 26 signals at this point is quite a lot, but it does need some adjustment.

snowleopard commented 7 years ago

I think formatting is wrong, perhaps this will work:

instance Show Signal where
    show (Signal i)
        | i < 26    = [chr (ord 'A' + i)]
        | otherwise = 'S' : show i
jrbeaumont commented 7 years ago

That works, thanks. Next issue:

This is a bit sad: "+"isSuffixOff. Can't you convert to strings later, so you can avoid testing strings instead of working with Transition type?

Because of the numbering of transitions (e.g c+/1), I can't change the first argument to transition, (String, String) to (Transition Signal, Transition Signal). I can change it to (Transition Signal, String) which would remove "+" isSuffixOf.

jrbeaumont commented 7 years ago

Made these changes now. Let me know if there's more to be done :)

snowleopard commented 7 years ago

Merged from 10,600m above.