Repository navigation
[spectec] Update meta-interpreter for descriptors #128
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,9 +93,16 @@ let split_iter s = | |
| String.sub s 0 last, String.sub s last (len - last) | ||
|
|
||
| let replace_name old_name new_name algo = | ||
| let replace_name' walker e = | ||
| let replace_name' (walker: Walk.walker) e = | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI explanation of why this change is necessary:
Someone with more context will have to say whether that makes sense.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds plausible at least. |
||
| match e.it with | ||
| | VarE x when x = old_name -> {e with it = VarE new_name} | ||
| | IterE (inner_e, (iter, xes)) -> | ||
| let xes' = | ||
| List.map | ||
| (fun (x, e) -> (if x = old_name then new_name else x), walker.walk_expr walker e) | ||
| xes | ||
| in | ||
| {e with it = IterE (walker.walk_expr walker inner_e, (walker.walk_iter walker iter, xes'))} | ||
| | _ -> Walk.base_walker.walk_expr walker e | ||
| in | ||
| let walker = {Walk.base_walker with walk_expr = replace_name'} in | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ struct | |
| include Print | ||
| end | ||
|
|
||
| let version = ref 3 | ||
| let version = ref 4 | ||
|
|
||
|
|
||
| (* Errors *) | ||
|
|
||
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 is a bit of a hack. wasm-latest still mirrors wasm-3.0 and they use the same test directory, but wasm-3.0 cannot run the custom descriptors tests. Maybe we need to start mirroring the tests into a separate "latest" directory that proposals can update, similar to what we do for the .spectec sources.
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.
Yes, good point. Though unlike with the specs, we do not keep the old versions of the test suite around (maybe we should?). So it's less clear what should happen when merging the feature.
Alternatively, instead of hard-coding a single test name inline here, perhaps add some kind of blacklist that is easier to update?
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'll land this for now, but I'm happy to adopt a different scheme here if you decide on one. I don't have a strong feeling about what is best. I guess it's not the end of the world if each new proposal just adds this line to skip its own tests if necessary.