Add preliminary support for trait in parser and typechecker. #186
Conversation
|
Maybe it would be better to keep the trait development in a separate branch until it is fully ready to be merged into master? I'm happy to review it though. |
|
My concern is that the final PR could be too big to be properly reviewed, so I creating PRs whenever some milestone is reached. |
|
But you could still create pull requests to a branch (say |
| extractTypes (Program _ _ _ funs classes) = | ||
| List.nub $ concat $ concatMap extractFunctionTypes funs ++ concatMap extractClassTypes classes | ||
| extractTypes (Program{functions, classes}) = | ||
| List.nub $ concat $ concatMap extractFunctionTypes functions ++ concatMap extractClassTypes classes | ||
| where |
There was a problem hiding this comment.
It's good that these kind of refactorings are done!
There was a problem hiding this comment.
It's actually not refactoring. The original way does not build if I add more fields to Program. Using record syntax seems remove that constraint.
There was a problem hiding this comment.
Whatever you want to call it, I think using the record syntax is better.
|
I have pushed another commit based on the feedback. I could squeeze those two commits into one before merging if someone prefers that. |
|
|
||
| isMainMethod :: MonadReader Environment m => MethodDecl -> m Bool | ||
| isMainMethod method = isInMain >>= return . (&& (mname method == Name "main")) | ||
|
|
There was a problem hiding this comment.
Sorry for being picky, but I don't like that these are in Environment.hs. First of all, the rest of the functions are not monadic but treat the environment as an argument. Secondly, isMainMethod is breaking abstraction by letting the environment be aware of the special names this and main. Even though these names will probably never change, I think the environment should be agnostic of such things. Here is a suggestion patch for a version that keeps abstraction:
https://gist.github.com/EliasC/2dbebda951e302a9f18f
Feel free to use this, modify it, or argue why I'm wrong :)
There was a problem hiding this comment.
Makes sense. Updated.
|
Looking really good now (although I'll be back on the snake_case...)! Merging now. |
Add preliminary support for trait in parser and typechecker.
No description provided.