-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add iospec
#51
base: master
Are you sure you want to change the base?
Add iospec
#51
Conversation
f57dd61
to
748c66a
Compare
The derive #[derive(Default)] on enums is still unstable (should be stable from 1.62).
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.
From great PR comes great reviews
Overall amazing job!
I've left quite a few comments, most of which are either questions or small suggestions you can apply directly from GitHub. There are some major questions that may go unnoticed, which are:
- Can you describe further what do you mean by "Compile", "Eval", "EvalMut" and "Run"? I know that from the spec you produce source code (is this the "compile" step?), but also read the input file for validating it (which step is this?)
- I assume the IR is also an interpreter of the spec for parsing the input file for the validation, how does it work? (i.e. which traits are involved in doing so?)
- How fast is this interpreter compared to the equivalent validator compiled from C++? (and also compared to the same written in Python, and with pypy)
- Can you describe how the
mem
module work? How is it used and what is it used for? - What does
State
contain?
@@ -14,6 +14,8 @@ use task_maker_rust::tools::task_info::main_task_info; | |||
use task_maker_rust::tools::typescriptify::main_typescriptify; | |||
use task_maker_rust::tools::worker::main_worker; | |||
|
|||
use task_maker_iospec::tools::*; |
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.
I don't really like wildcard imports in general, maybe here it's fine, but my feelings about them are not great.
Apart from the obvious "they bring more than what is actually needed", I dislike them because it makes reading the code without an IDE with "Go to definition" support pretty much impossible (especially when more wildcards are used in the same file).
I don't want to ban them (there are still few good use cases, like for "prelude"s), but I always try to avoid them.
(I mention this here, and skip all the following instances of it)
/// Check input/output files against a specification in the `iospec` language. | ||
IospecCheck(iospec_check::Opt), | ||
/// Generate graders or other I/O-related files given a specification in the `iospec` language. | ||
IospecGen(iospec_gen::Opt), | ||
/// Generate standard set of files given an I/O format specification in the `iospec` language. | ||
IospecGenAll(iospec_gen_all::Opt), |
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.
iospec-check Check input/output files against a specification in the `iospec`
language
iospec-gen Generate graders or other I/O-related files given a specification in
the `iospec` language
iospec-gen-all Generate standard set of files given an I/O format specification in
the `iospec` language
Nit: is it possible to rephrase them so that they won't get a line break with --help
?
@@ -0,0 +1,22 @@ | |||
[package] |
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.
GitHub doesn't allow me to place comments on submodules, so I write it here.
task-maker-exec/tabox
has been added as a submodule. Why? Is it really needed or left from previous iterations?
[dependencies] | ||
syn = { version = "1.0.92", features = ["extra-traits"] } | ||
proc-macro2 = { version = "1.0.19", features = ["span-locations"] } | ||
annotate-snippets = { version = "0.9.0", features = ["color"] } | ||
codemap = "0.1.3" | ||
num-traits = "0.2.12" | ||
by_address = "1.0.4" | ||
anyhow = "1.0.57" | ||
clap = { version = "3.1", features = ["derive"] } | ||
|
||
[dev-dependencies] | ||
assert_cmd = "2.0.4" | ||
goldenfile = "1.1.0" | ||
tempdir = "0.3.7" | ||
tempfile = "3.3.0" | ||
walkdir = "2.3.2" |
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.
I usually document (with a super short comment) what each dependency do. For example:
@@ -0,0 +1,2 @@ | |||
unstable_features = true | |||
imports_granularity = "Item" |
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.
I usually use module, but done by hand. And I don't usually force it (especially with unstable features).
.arg("solution.c") | ||
.arg("-o") | ||
.arg("main.c.bin") | ||
// FIXME: missing `free` in generated C |
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.
To do. Or maybe just ignore memory leaks in AddressSanitizer.
LangOpt::Cpp => "cpp", | ||
LangOpt::C => "c", | ||
LangOpt::Inspect => "inspect", | ||
// LangOpt::Tex => "tex", |
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.
Commented code.
|| { | ||
for call in main.inner.calls.iter() { | ||
gen!(ctx, { | ||
"std::function<{}({})> {} = [](auto...) {{{}}};" |
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.
Why is this std::function
instead of a plain function pointer?
#[derive(Debug, Clone)] | ||
pub struct CallMetaStmt { | ||
pub kw: kw::call, | ||
pub name: Name, |
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.
What if you have two @call
statements for the same function name? Does it throw a nice error? I don't think we want to support function overloading.
} | ||
} | ||
|
||
// @call init(a=a); |
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.
Why are all the @call
in this spec commented out? Is this the reason why the solution files are empty?
High level comment: why don't we leave figuring out of formatting to appropriate language-specific tooling? (i.e. clang-format, autopep8 or whatever) |
iospec
is a tool to generate parsers for, validate and transform input/output files, given a specification of the format in an expressive language, as well as to generate documentation of the format itself.Goals
What's implemented already?
rustc
-like diagnostic messages for most errors when parsing a specProposed new API / contact points for problem developers
gen/IOSPEC
file with description of input formats, assumptions (incl. subtask-specific ones), and solution interface,task-maker
command (and/or adding one or more newtask-maker-tools
commands) to generate the following files fromgen/IOSPEC
(if given):sol/template.{ext}
for every configured and supported language,sol/grader.{ext}
for every configured and supported language,gen/genlib.hpp
, support lib for C++ generators, validators and checkers (see below),statement/lib.tex
, TeX macros for statements (see below),statement/messages.{lang}.tex
, TeX macros with translatable text (see below),task-maker-tools iospec-check
(once),gen/IOSPEC
, and stops if any if found,task-maker-tools iospec-check <input.txt> --cfg subtask_name=<name>
,gen/IOSPEC
,task-maker-tools iospec-check <input.txt> <output.txt> --cfg subtask_name=<name>
on every input/output,genlib.hpp
:struct IoData { ... }
with public members corresponding to I/O data,GENERATOR_MAIN(function1, function2, ...)
macro to use in generators,function1
,function2
, etc. accept an arbitrary number of parameters and returnIoData
,function1
, parameters are parsed automatically fromargv
,argv[1]
is used to choose which of the functions to call, then as above,Rng
(API TBD) initialized from a seed,VALIDATOR_MAIN(validator_function)
macro to use in validators,validator_function
is avoid validator_function(IoData data);
VALIDATOR_GRADER()
macro to use in validators,CHECKER_MAIN(checker_function)
,checker_function
is avoid checker_function(IoData correct, IoData submission);
correct
andsubmission
,std::string get_subtask_name()
(or similar name, TBD) returning the current subtask namestatement/lib.tex
(in a future PR):messages.{lang}.tex
(e.g., a language-specific macro likefor every #1
used to generatefor every $i=0,\dots,N$:
)gen/IOSPEC
(TBD)