Conversation
nojb
left a comment
There was a problem hiding this comment.
LGTM. But as I mentioned to Mark, maybe ocamlc -i is a simpler way to go about it?
tools/make_opcodes.mll
Outdated
| end; | ||
| if !print_opcodes then | ||
| List.iteri (fun i op -> printf "let op%s = %i\n" op i) opnames | ||
| end |
There was a problem hiding this comment.
For symmetry with existing code, it would be better to write
if !print_opcodes_mli then
List.iteri ...;
if !print_opnames then begin
...
tools/make_opcodes.mll
Outdated
| "-opnames", Arg.Set print_opnames, " Dump opcode names"; | ||
| "-opcodes", Arg.Set print_opcodes, " Dump opcode numbers"; | ||
| "-opcodes-mli", Arg.Set print_opcodes_mli, | ||
| " Produce the .mli for opcodes.ml"; |
There was a problem hiding this comment.
For symmetry the flags should be names -opcodes-ml and -opcodes-mli.
tools/make_opcodes.mll
Outdated
| { | ||
| let print_opnames = ref false | ||
| let print_opcodes = ref false | ||
| let print_opcodes_mli = ref false |
There was a problem hiding this comment.
I would rename print_opcodes by print_opcodes_ml.
|
Shouldn't |
|
I think I'm going to try |
|
nojb
left a comment
There was a problem hiding this comment.
LGTM - I like the shorter patch better.
This pull request adds a .mli file for
bytecomp/opcodes.mli.This is needed to simplify a patch I will post shortly to improve the packing mechanism used to build the various
Dynlinklibraries. (It will mean that every.mlfile from compilerlibs thatDynlinkdepends on has a corresponding.mlifile.)In turn, that patch is needed for another patch I am preparing that overhauls the types used to represent various kinds of object file symbols throughout the compiler, which is in turn needed for
Asm_directives, which is in turn needed for the gdb support.