-
Notifications
You must be signed in to change notification settings - Fork 22
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
allow more letters in identifiers #333
Conversation
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.
LGTM, a few nitpicks.
@@ -27,7 +27,9 @@ let mk_config loc lid = | |||
| "CBN" when !strat = None -> strat := Some ByName | |||
| "CBV" when !strat = None -> strat := Some ByValue | |||
| "CBSV" when !strat = None -> strat := Some ByStrongValue | |||
| i when !nb_steps = None -> nb_steps := Some (int_of_string i) | |||
| i when !nb_steps = None -> | |||
let n = int_of_string i in |
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.
int_of_string
can raise a Failure
exception, you may want to catch it, don't you?
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.
Note that Failure was not specifically caught before either. It is caught a few lines later and generates a scoping error:
Dedukti/parsing/menhir_parser.mly
Line 38 in b7949df
with _ -> raise @@ Scoping_error(loc, "invalid command configuration") |
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.
Do you think that we should introduce a more specific error?
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'd say it depends if you really expect it to happen, or if it's an assert false
kind of failure (but at least you want to know where it came from as a developer, if it ever happens)
The main modifs are in parsing/lexer.mll and parsing/menhir_parser.mly.
Spaces are added in test files (e.g. writing A -> A instead of A->A).
The error code for tests/KO/nsteps1.dk (
#EVAL[-1]
) is changed in tests/main.ml from lexing error to scoping error because '-' is now allowed in identifiers but the argument of#EVAL
must be a non-negative integer.