Conversation
f57dd61 to
748c66a
Compare
The derive #[derive(Default)] on enums is still unstable (should be stable from 1.62).
edomora97
left a comment
There was a problem hiding this comment.
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
memmodule work? How is it used and what is it used for? - What does
Statecontain?
| 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.
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.
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.
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.
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.
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.
To do. Or maybe just ignore memory leaks in AddressSanitizer.
| LangOpt::Cpp => "cpp", | ||
| LangOpt::C => "c", | ||
| LangOpt::Inspect => "inspect", | ||
| // LangOpt::Tex => "tex", |
| || { | ||
| for call in main.inner.calls.iter() { | ||
| gen!(ctx, { | ||
| "std::function<{}({})> {} = [](auto...) {{{}}};" |
There was a problem hiding this comment.
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.
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.
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) |
iospecis 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/IOSPECfile with description of input formats, assumptions (incl. subtask-specific ones), and solution interface,task-makercommand (and/or adding one or more newtask-maker-toolscommands) 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_functionis avoid validator_function(IoData data);VALIDATOR_GRADER()macro to use in validators,CHECKER_MAIN(checker_function),checker_functionis avoid checker_function(IoData correct, IoData submission);correctandsubmission,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 #1used to generatefor every $i=0,\dots,N$:)gen/IOSPEC(TBD)