Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Primitive port and node construction #45

Merged
merged 39 commits into from
Jan 20, 2015

Conversation

mtrberzi
Copy link
Member

@mtrberzi mtrberzi commented Dec 7, 2014

I would like to submit this pull request for review, knowing that I am not done with it yet. There is a lot of code here, but finishing all of the features I have marked as TODO in the code would result in a PR of several thousand lines, and this is bad enough already.

  • Yes, this finally gets rid of the old digital circuit primitive definitions. Those are going to be gone permanently after this, because we can now define port types and node types in the frontend language.
  • This also removes all of the old "functionality" of ExpressionGraph, which made horrible assumptions about the structure of the schematic that was being compiled and wouldn't work for anything except digital circuits.
  • TypeChecker is completely gone. All typing can be done dynamically through the expression graph, which has all the benefits of a static system while providing dynamic typing only where it is actually used.
  • Test coverage is down quite a bit, but I plan to add some test cases to this branch after it is reviewed and hopefully get the coverage even higher than it is now.
  • There are many TODOs hanging around in the code. Some of them are easy enough to resolve before this is merged; some of them are large enough to merit opening a separate issue.

Conflicts:
	build.gradle
	src/main/java/org/manifold/compiler/front/ExpressionGraph.java
	src/main/java/org/manifold/compiler/front/FrontendValueVisitor.java
(removes most of ExpressionGraph, so WIP.)

public class PrimitivePortDefinitionExpression extends Expression {

// TODO(murphy) figure out where the name of the port type value comes from
Copy link
Contributor

Choose a reason for hiding this comment

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

🐶

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving it since this is going away soon

@lucaswoj
Copy link
Contributor

lucaswoj commented Jan 7, 2015

Done my first review. Sorry about the zoo.


public class ExpressionGraphBuilder implements ExpressionVisitor {

private static Logger log = LogManager.getLogger("ExpressionGraph");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ExpressionGraphBuilder.class?

@maxqchen
Copy link
Contributor

Logic looks good (AFAIK anyways, still a bit fuzzy on the whole thing), just a few minor nit picky things 🚲.

@mtrberzi
Copy link
Member Author

Fantastic first round of review, everyone! Thanks again. New commits are available.

@coveralls
Copy link

Coverage Status

Coverage decreased (-20.15%) when pulling 8786e67 on mtrberzi:primitive-definitions into 3d9b44a on manifold-lang:master.

mtrberzi added a commit that referenced this pull request Jan 20, 2015
Primitive port and node construction
@mtrberzi mtrberzi merged commit 1501190 into manifold-lang:master Jan 20, 2015
@mtrberzi mtrberzi deleted the primitive-definitions branch January 20, 2015 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants