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

Implemented a basic compiler that integrates lexer and parser + assembler directives explained #4

Merged
merged 12 commits into from
Jan 29, 2024

Conversation

Fiwo735
Copy link
Collaborator

@Fiwo735 Fiwo735 commented Jan 27, 2024

As discussed over MS Teams.

Copy link
Collaborator

@Jpnock Jpnock left a comment

Choose a reason for hiding this comment

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

Great work here! This was certainly needed as previously you could spend anywhere from five to ten hours trying to get this framework setup yourself, just to get to a point where you could add a single feature to your compiler. Collectively this will easily save hundreds of hours across the cohort :D

I've made some suggestions which can be summed up as:

  • More Makefile improvements (track dependencies more generically, use GNU bison instead of yacc & also depend on the Makefile itself)
  • Pass the Context by reference instead of by copy

Additionally, I'd maybe take a look at the int f() { return; } example as this leads to undefined behaviour; this is as the value of f() is used by example_driver.c when running ./test.sh. So this needs to be reworked I think (maybe back to the original return 5, even if it does mean having to add a Number/IntegerConstant class). I've put some more details on the line itself.

src/compiler.cpp Show resolved Hide resolved
include/ast_node.hpp Outdated Show resolved Hide resolved
include/ast_jump_statement.hpp Outdated Show resolved Hide resolved
include/ast_identifier.hpp Outdated Show resolved Hide resolved
include/ast_function_definition.hpp Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
include/ast_identifier.hpp Show resolved Hide resolved
Fiwo735 and others added 2 commits January 29, 2024 06:08
@Fiwo735
Copy link
Collaborator Author

Fiwo735 commented Jan 29, 2024

Thanks for the review James, I really appreciate it! Saving hundreds of hours across the cohort was indeed my biggest motivation!

Re Makefile - brilliant changes, setting makefiles is not my strong point, but your version makes much more sense (and hopefully is even less error-prone for the students).
Re Context - passing by reference was indeed my plan, but I simply mistyped it once and then copy-pasted wrongly in each place... thanks for spotting that!

I've left some minor comments in some of your suggestions above and committed the rest of them.

@johnwickerson johnwickerson merged commit 5e9c533 into LangProc:main Jan 29, 2024
1 check failed
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.

3 participants