Skip to content
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 bytecomp/opcodes.mli #2265

Merged
merged 2 commits into from Feb 26, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Feb 26, 2019

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 Dynlink libraries. (It will mean that every .ml file from compilerlibs that Dynlink depends on has a corresponding .mli file.)

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.

@mshinwell mshinwell requested a review from nojb Feb 26, 2019
Copy link
Contributor

left a comment

LGTM. But as I mentioned to Mark, maybe ocamlc -i is a simpler way to go about it?

end;
if !print_opcodes then
List.iteri (fun i op -> printf "let op%s = %i\n" op i) opnames
end

This comment has been minimized.

Copy link
@nojb

nojb Feb 26, 2019

Contributor

For symmetry with existing code, it would be better to write

if !print_opcodes_mli then
  List.iteri ...;
if !print_opnames then begin
  ...
@@ -35,16 +36,22 @@ and opnames = parse
[
"-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";

This comment has been minimized.

Copy link
@nojb

nojb Feb 26, 2019

Contributor

For symmetry the flags should be names -opcodes-ml and -opcodes-mli.

@@ -27,6 +27,7 @@ and opnames = parse
{
let print_opnames = ref false
let print_opcodes = ref false
let print_opcodes_mli = ref false

This comment has been minimized.

Copy link
@nojb

nojb Feb 26, 2019

Contributor

I would rename print_opcodes by print_opcodes_ml.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Shouldn't names_of_... also show up in the .mli when its generation is enabled? (I don't remember what this is used for.)

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

I think I'm going to try ocamlc -i instead, although in general, I think that generators should write the interface and the implementations themselves.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Shouldn't names_of_... also show up in the .mli when its generation is enabled? (I don't remember what this is used for.)

names_of_... is for different file, tools/opnames.ml, not bytecomp/opcodes.ml.

@nojb
nojb approved these changes Feb 26, 2019
Copy link
Contributor

left a comment

LGTM - I like the shorter patch better.

@mshinwell mshinwell merged commit 3133699 into ocaml:trunk Feb 26, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.