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

Improve tools/read_cmt #1036

Merged
merged 1 commit into from May 11, 2017

Conversation

Projects
None yet
3 participants
@lefessan
Contributor

lefessan commented Feb 10, 2017

We use read_cmt to provide .annot files when developing in a version of the compiler for which merlin or ocp-index are not available. This PR provides some additional features that are needed by our tool.

  • Install read_cmt as ocaml_cmt (in native-code if possible)
  • Print cmt info in file when -o is specified
  • Add an option -I for includes
  • Catch and print exceptions
@dra27

I don't have observations on the changes for module bindings, but a few thoughts on the installation and the new parameters.

| Packed _ ->
Printf.fprintf stderr "Packed files not yet supported\n%!";
Stypes.dump target_filename
| Partial_interface _ -> assert false

This comment has been minimized.

@dra27

dra27 Feb 18, 2017

Contributor

I realise there are differences of opinion on how match blocks should be indented, but this would be considerably easier to review if the new match case followed the 4 spaces indentation of the existing clauses, rather than altering everything (as it happens, I think the existing indentation is better, as it more clearly differentiates the match clauses from the blocks of code).

"-info", Arg.Set print_info_arg, " : print information on the file";
"-args", Arg.Expand Arg.read_arg,
" <file> Read additional newline separated command line arguments \n\
\ from <file>";
"-args0", Arg.Expand Arg.read_arg0,
"<file> Read additional NUL separated command line arguments from \n\
\ <file>";
"-I", Arg.String (fun s ->
Clflags.include_dirs := s :: !Clflags.include_dirs
), "DIR Directories for includes";

This comment has been minimized.

@dra27

dra27 Feb 18, 2017

Contributor

Couldn't this be worded more like driver/main_args.ml (<dir> Add <dir> to the list of include directories)

| None -> ()
| Some _ -> close_out oc
end;
()

This comment has been minimized.

@dra27

dra27 Feb 18, 2017

Contributor

Same whitespace comment - how is this supposed to be reviewed?!

@@ -281,6 +281,13 @@ READ_CMT= \
# Reading cmt files
$(call byte_and_opt,read_cmt,$(READ_CMT),)
install::
if test -f read_cmt.opt; then \

This comment has been minimized.

@dra27

dra27 Feb 18, 2017

Contributor

This should install a .byte version (as other tools are now) rather than just installing one - or for newly installed tools do we not care for both?

@@ -281,6 +281,13 @@ READ_CMT= \
# Reading cmt files
$(call byte_and_opt,read_cmt,$(READ_CMT),)
install::
if test -f read_cmt.opt; then \
cp read_cmt.opt "$(INSTALL_BINDIR)/ocaml_cmt$(EXE)"; \

This comment has been minimized.

@dra27

dra27 Feb 18, 2017

Contributor

I'm not convinced by the name - ocaml-cmt2annot or ocaml-cmtutil perhaps?

@mshinwell

This comment has been minimized.

Contributor

mshinwell commented May 4, 2017

@lefessan Could you address @dra27 's comments so this can be considered for merging?

Fix tools/read_cmt
Install tools/read_cmt as ocamlcmt
* Add option -save-cmt-info to add more info in .annot files
* Provide more annotations
* Add option -I <dir> to find .cmi files
@lefessan

This comment has been minimized.

Contributor

lefessan commented May 10, 2017

Here is a rebased version:

  • Better help for arguments (as suggested by @dra27)
  • The tool is now installed as ocamlcmt instead of ocaml-cmt
  • Better indentation (although I changed the indentation on a large part because of let open X in that was previously wrongly indented)

@lefessan lefessan merged commit fb2a442 into ocaml:trunk May 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lefessan lefessan deleted the lefessan:2017-02-10-cmt_read branch May 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment