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

Register all idents relevant for reraise. #1567

Merged
merged 2 commits into from Feb 19, 2018

Conversation

Projects
None yet
4 participants
@trefis
Copy link
Contributor

trefis commented Jan 15, 2018

Ping @alainfrisch ? (as you were the author of c4513e1)

in
aux c_lhs
in
IdentSet.iter (fun id -> Hashtbl.replace try_ids id ()) ids_to_remember;

This comment has been minimized.

@alainfrisch

alainfrisch Jan 15, 2018

Contributor

More a matter of taste, but one could avoid materializing the set and instead pass a callback (replace/remove) to aux.

print_string "d"; print_newline(); raise exn
| exception (Error "e" as exn as exn2) ->
(* this should Re-raise, appending to the current backtrace *)
print_string "e"; print_newline(); raise exn2

This comment has been minimized.

@alainfrisch

alainfrisch Jan 15, 2018

Contributor

Perhaps add a test of the form "exn as exn2".

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Jan 15, 2018

LGTM, modulo tiny comments (which you can ignore if you wish), and a required Changes entry.

@trefis trefis force-pushed the trefis:reraise branch from 1d2d89d to 3cb251f Jan 15, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Jan 15, 2018

Thanks for the quick review.
I added the test you suggested and changed transl_case following your comment.

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Jan 15, 2018

Appveyor isn't happy:

List of failed tests:
    tests/backtrace/'backtrace3.ml' with ocamlc

But I have no idea why

  • it's happy with ocamlopt but not ocamlc
  • travis is happy with ocamlc but appveyor is not

Ping @dra27 ?

@dra27
Copy link
Contributor

dra27 left a comment

I'm looking into the AppVeyor failure, two minor nits at Warning 26 being displayed on the console...

@@ -21,7 +21,24 @@ let g msg =
| exception (Error "c") ->
(* according to the current re-raise policy (a static condition),
this does not re-raise *)
raise (Error "c")
print_string "c"; print_newline(); raise (Error "c")
| exception (Error "d" as exn as exn2) ->

This comment has been minimized.

@dra27

dra27 Jan 17, 2018

Contributor

I think the test still makes sense if this is _exn2, which should eliminate Warning 26

This comment has been minimized.

@trefis

trefis Jan 17, 2018

Author Contributor

That sounds right yes, I'll fix that.

| exception (Error "d" as exn as exn2) ->
(* this should Re-raise, appending to the current backtrace *)
print_string "d"; print_newline(); raise exn
| exception (Error "e" as exn as exn2) ->

This comment has been minimized.

@dra27

dra27 Jan 17, 2018

Contributor

Similarly here _exn

Re-raised at file "backtrace3.ml", line 38, characters 45-55
Called from file "backtrace3.ml", line 45, characters 11-23
Uncaught exception Backtrace3.Error("h")
Raised at file "backtrace3.ml", line 41, characters 10-17

This comment has been minimized.

@dra27

dra27 Jan 17, 2018

Contributor

I've reproduced the AppVeyor error. It's nothing to do with CRLF (which is not surprising, because that would be very strange for it to affect bytecode only). The character reference is 16-17 for bytecode. A little tweaking shows that it's precisely returning the location of x not raise x

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Jan 17, 2018

I suppressed warning 26 as @dra27 suggested and rebased on top of #1573 so the tests should now pass.

@let-def

This comment has been minimized.

Copy link
Contributor

let-def commented Feb 16, 2018

The ~is_exn_case doesn't look like a win: you share one more line of code (List.filter (fun c -> c.c_rhs.exp_desc <> Texp_unreachable) cases), but you have to pass the flag down to the translate function and immediately test it... Not worth it :P.

Otherwise, this is good.

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Feb 16, 2018

I was about to say:

Yes, the idea wasn't to factorise code so much as to force people to explicitly think about what they are doing when calling this function.
IIRC, I did this because my initial implementation of #1568 was calling transl_case when it should have been calling transl_case_try.

But then I noticed that (probably out of lazyness) I made the flag optional. Which kind of defeats the purpose.
I'm happy to either revert that change, make the flag non optional, or just do nothing.

What do you think?

@trefis trefis force-pushed the trefis:reraise branch from d2d2f36 to 9b4391c Feb 19, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Feb 19, 2018

Reverted the changes @let-def pointed out.

@trefis trefis merged commit 59a8d4d into ocaml:trunk Feb 19, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@trefis trefis deleted the trefis:reraise branch Feb 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.