-
Notifications
You must be signed in to change notification settings - Fork 21
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
Renamed cli.h into cli.hpp, used structured bindings, made cli and compiler more consistent #24
Open
Fiwo735
wants to merge
3
commits into
dev
Choose a base branch
from
cli_changes
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
#include <cli.h> | ||
#include <cli.hpp> | ||
|
||
CommandLineArguments ParseCommandLineArgs(int argc, char **argv) | ||
{ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,69 +1,78 @@ | ||
#include <fstream> | ||
#include <iostream> | ||
|
||
#include "cli.h" | ||
#include "cli.hpp" | ||
#include "ast.hpp" | ||
|
||
using ast::NodePtr; | ||
|
||
NodePtr Parse(const CommandLineArguments& args); | ||
// Wrapper for ParseAST defined in YACC | ||
NodePtr Parse(const std::string& compile_source_path); | ||
|
||
// Output the pretty print version of what was parsed to the .printed output | ||
// file. | ||
void PrettyPrint(const NodePtr& root, const CommandLineArguments& args); | ||
// Output the pretty print version of what was parsed to the .printed output file. | ||
void PrettyPrint(const NodePtr& root, const std::string& compile_output_path); | ||
|
||
// Compile from the root of the AST and output this to the | ||
// args.compiledOutputPath file. | ||
void Compile(const NodePtr& root, const CommandLineArguments& args); | ||
// Compile from the root of the AST and output this to the compiledOutputPath file. | ||
void Compile(const NodePtr& root, const std::string& compile_output_path); | ||
|
||
int main(int argc, char **argv) | ||
{ | ||
// Parse CLI arguments to fetch the source file to compile and the path to output to. | ||
// This retrives [source-file.c] and [dest-file.s], when the compiler is invoked as follows: | ||
// ./bin/c_compiler -S [source-file.c] -o [dest-file.s] | ||
auto command_line_arguments = ParseCommandLineArgs(argc, argv); | ||
const auto [compile_source_path, compile_output_path] = ParseCommandLineArgs(argc, argv); | ||
Jpnock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Parse input and generate AST | ||
auto ast_root = Parse(command_line_arguments); | ||
// Parse input and generate AST. | ||
auto ast_root = Parse(compile_source_path); | ||
|
||
// Check something was actually returned by parseAST(). | ||
if (ast_root == nullptr) | ||
{ | ||
// Check something was actually returned by parseAST(). | ||
std::cerr << "The root of the AST was a null pointer. Likely the root was never initialised correctly during parsing." << std::endl; | ||
std::cerr << "The root of the AST is a null pointer. "; | ||
std::cerr << "Likely the root was never initialised correctly during parsing." << std::endl; | ||
return 3; | ||
} | ||
|
||
PrettyPrint(ast_root, command_line_arguments); | ||
Compile(ast_root, command_line_arguments); | ||
// Print AST in a human-readable way. It's not assessed, but exists for your convenience. | ||
PrettyPrint(ast_root, compile_output_path); | ||
|
||
// Compile to RISC-V assembly, the main goal of this project. | ||
Compile(ast_root, compile_output_path); | ||
} | ||
|
||
NodePtr Parse(const CommandLineArguments& args) | ||
NodePtr Parse(const std::string& compile_source_path) | ||
{ | ||
std::cout << "Parsing: " << args.compile_source_path << std::endl; | ||
NodePtr root = ParseAST(args.compile_source_path); | ||
std::cout << "Parsing ..." << compile_source_path << std::endl; | ||
|
||
NodePtr root = ParseAST(compile_source_path); | ||
|
||
std::cout << "AST parsing complete" << std::endl; | ||
|
||
return root; | ||
} | ||
|
||
void PrettyPrint(const NodePtr& root, const CommandLineArguments& args) | ||
void PrettyPrint(const NodePtr& root, const std::string& compile_output_path) | ||
{ | ||
auto output_path = args.compile_output_path + ".printed"; | ||
auto output_path = compile_output_path + ".printed"; | ||
|
||
std::cout << "Printing parsed AST..." << std::endl; | ||
|
||
std::ofstream output(output_path, std::ios::trunc); | ||
root->Print(output); | ||
output.close(); | ||
|
||
std::cout << "Printed parsed AST to: " << output_path << std::endl; | ||
} | ||
|
||
void Compile(const NodePtr& root, const CommandLineArguments& args) | ||
void Compile(const NodePtr& root, const std::string& compile_output_path) | ||
{ | ||
// Create a Context. This can be used to pass around information about | ||
// what's currently being compiled (e.g. function scope and variable names). | ||
ast::Context ctx; | ||
|
||
std::cout << "Compiling parsed AST..." << std::endl; | ||
std::ofstream output(args.compile_output_path, std::ios::trunc); | ||
|
||
std::ofstream output(compile_output_path, std::ios::trunc); | ||
root->EmitRISC(output, ctx); | ||
output.close(); | ||
std::cout << "Compiled to: " << args.compile_output_path << std::endl; | ||
|
||
std::cout << "Compiled to: " << compile_output_path << std::endl; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bit too extra, but it might be worth considering using
std::filesystem::path
rather than strings? This has the benefit of making it clear that this variable represents a file path from its type rather than its name, which is usually an improvement imo - but if you think it's not worth the complexity / overhead for students happy for you to keep it as is. I think that this coursework is a good opportunity for us to teach students about the features of modern C++, but defs think we need to pick our battles with what we want to include (and smart pointers might be enough of a step for them 😅)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another interesting choice between teaching modern C++ vs not scaring students too much. I can see both points being made here and I'm slightly in favor of including this change as it doesn't look too complex and should be understandable from the type name itself. Any strong opinions @Jpnock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me