search

manifold-lang / manifold-frontend

Frontend compiler for Manifold high-level language
GNU General Public License v3.0
6 stars 6 forks source link

Build expression graphs directly from the parse tree #65

Closed mtrberzi closed 9 years ago

mtrberzi commented 9 years ago

This is ready to go as is, but I can probably remove even more classes now that lots of the Expression stuff has gone away.

mtrberzi commented 9 years ago

Fixes #48

coveralls commented 9 years ago

Coverage Status

Coverage increased (+40.11%) to 58.72% when pulling fa7e63cc2b7b080d709f7e884856a2c191d8eb1e on mtrberzi:issue48-graphbuilding into fd363f9fdf1545741a3faba218b7e911665f392d on manifold-lang:master.

lucaswoj commented 9 years ago

This is ready to go as is, but I can probably remove even more classes now that lots of the Expression stuff has gone away.

Sweet! Want a review now or are you going to keep working on this PR?

mtrberzi commented 9 years ago

Feel free to review it now. The only other work is going to be deleting a bunch of unused classes.

lucaswoj commented 9 years ago

@mtrberzi Any word on when you can update this with the deleted classes?

mtrberzi commented 9 years ago

It turned out to be a bigger mess than I thought, because I keep getting the dependencies wrong. Can we open an issue for it and merge this as-is?

lucaswoj commented 9 years ago

I'd rather you fully complete this task before merging the pull request. If you are still finding dependencies, that would suggest the issue is not yet resolved.

On Fri, Mar 13, 2015 at 3:28 PM, Murphy Berzish notifications@github.com wrote:

It turned out to be a bigger mess than I thought, because I keep getting the dependencies wrong. Can we open an issue for it and merge this as-is?

Reply to this email directly or view it on GitHub: https://github.com/manifold-lang/manifold-frontend/pull/65#issuecomment-79282468

mtrberzi commented 9 years ago

Fair -- I'll review it later and see what I can do.

On Fri, Mar 13, 2015 at 3:40 PM, Lucas Wojciechowski < notifications@github.com> wrote:

I'd rather you fully complete this task before merging the pull request. If you are still finding dependencies, that would suggest the issue is not yet resolved.

On Fri, Mar 13, 2015 at 3:28 PM, Murphy Berzish notifications@github.com wrote:

It turned out to be a bigger mess than I thought, because I keep getting

the dependencies wrong. Can we open an issue for it and merge this as-is?

Reply to this email directly or view it on GitHub:

https://github.com/manifold-lang/manifold-frontend/pull/65#issuecomment-79282468

— Reply to this email directly or view it on GitHub https://github.com/manifold-lang/manifold-frontend/pull/65#issuecomment-79294605 .

coveralls commented 9 years ago

Coverage Status

Coverage increased (+43.93%) to 62.53% when pulling 5b4734d5b797320c2209b67e839c8f1965d52cc0 on mtrberzi:issue48-graphbuilding into fd363f9fdf1545741a3faba218b7e911665f392d on manifold-lang:master.

mtrberzi commented 9 years ago

New commits are available.

lucaswoj commented 9 years ago 
+292 additions
−1,141 deletions

Thats what I like to see! Great work @mtrberzi :fireworks:

mtrberzi commented 9 years ago

Thanks for the review, new commits are available.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+44.11%) to 62.72% when pulling e22fcbf0e459d92d824569010da8c5a914bebf82 on mtrberzi:issue48-graphbuilding into fd363f9fdf1545741a3faba218b7e911665f392d on manifold-lang:master.

maxqchen commented 9 years ago

Tyson pointed out 2 instances of Integer nextAnonymousID = 0; declared in the wrong scope, but the new diff only moved 1 out. Other than that looks okay to me. The try blocks still seem kind of awkward but not that big of a deal.

LGTM after the nextAnonymousId fix.

mtrberzi commented 9 years ago

Thanks, got it.

On Sun, Mar 15, 2015 at 4:16 PM, Max Qitian Chen notifications@github.com wrote:

Tyson pointed out 2 instances of Integer nextAnonymousID = 0; declared in the wrong scope, but the new diff only moved 1 out. Other than that looks okay to me. The try blocks still seem kind of awkward but not that big of a deal.

LGTM after the nextAnonymousId fix.

— Reply to this email directly or view it on GitHub https://github.com/manifold-lang/manifold-frontend/pull/65#issuecomment-81222878 .

coveralls commented 9 years ago

Coverage Status

Coverage increased (+44.11%) to 62.72% when pulling 3893b7d38a4fdb9e5b61d66a450c276aaa109961 on mtrberzi:issue48-graphbuilding into fd363f9fdf1545741a3faba218b7e911665f392d on manifold-lang:master.

maxqchen commented 9 years ago

Cool. :shipit: (assuming no one else has any issues with it).

lucaswoj commented 9 years ago

Ship it

On Mon, Mar 16, 2015 at 2:44 PM, Max Qitian Chen notifications@github.com wrote:

Cool. :shipit: (assuming no one else has any issues with it).

Reply to this email directly or view it on GitHub: https://github.com/manifold-lang/manifold-frontend/pull/65#issuecomment-81861801

TysonAndre commented 9 years ago

:ship: