Renamed cli.h into cli.hpp, used structured bindings, made cli and compiler more consistent #24
Conversation
There was a problem hiding this comment.
LGTM. I've approved but left some comments about structured binding being used in this case.
Originally my reasoning for passing the struct around was so it was obvious where these values were coming from. That being said, this file is small enough to skim through in a few seconds so I agree with the changes here.
|
Also I like the |
Co-authored-by: Jpnock <[email protected]>
| #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); |
There was a problem hiding this comment.
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.
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.
but it might be worth considering using std::filesystem::path
Sounds good to me
Just a small PR to clear some inconsistencies:
cli.hremaned tocli.hppto match other header files.command_line_arguments, leading to clearer intent and removed the need to pass the whole struct into functions.compiler.cpp, as that's where most students would look first.Note the branch name changes, so that we now have:
maindev: all ongoing development, especially useful when the coursework is in progress<feature>: fresh branch created offdevbranch each time, leading to a new PR