This seems more engineered than it needs to be - with the effect at the moment that it's lost the name of the printer. Does Printexc.to_string not work in this context (so why can't _exn just be passed to exn_printer)?

Link to Mantis PR

Thanks for the prompt review. Printexc.to_string indeed works, I feel a bit silly that I couldn't find it. I'll push an update in a couple minutes.

Even with OCaml's relatively small standard library, it can be hard to see the wood for the trees sometimes!

However, now I can't make up my mind whether I preferred it saying Match_failure ("//toplevel//", 1, 18) before or the output with printers which Printexc.to_string gives! Quick opinion, @gasche?

@dra27 I am not sure exactly which output you have in mind, would you show an example? (Generally I would be tempted to not be super-picky: if we don't know which one we prefer, and nobody is going to be strongly negatively affected by the change, then maybe either is fine.)

I wonder what is the behavior when the value on which the exception is raised occurs in depth. Is the rest of the value printed correctly, or does the exception abort all printing? Could we have the testcase (a new one or modify the proposed one) to exhibit this behavior?

@gasche The message before was the value itself (Match_failure ("//toplevel//", 1, 18)) but of course Printexc.to_string uses printers so it now becomes File "//toplevel//", line 1, characters 18-23: Pattern matching failed.

I think it's probably the case that using printers is better?

@gasche The behaviour is to propagate the < ... > string. e.g.

type t = A | B;;
let print_t f = function A -> Format.fprintf f "A";;
#install_printer print_t;;
type u = X of t | Y;;
X B;;
- : u = X <printer print_t raised an exception: ...>

Given that this only alters the string (rather than changing the propagation of the exception), is it really worth a test case too?

I would just use this as a testcase instead of the proposed one, because it is strictly more informative. (Test cases are also used to let people that review patches quickly get a sense of what the changes does, without testing it by themselves.)

Re. using the printers: I agree using the printers is better. It seems to be the simplest approach to implement anyway.

@gasche OK, while we were chatting Tadeu has pushed the test!

@tadeuzagallo This looks great, thanks - as soon as the CI returns I'll merge.

Indeed, thanks for the nice patch -- and @dra27 for the review work. @dra27, as you merge, will you also close the Mantis PR? (I regularly forget to.)

The only issue I'm seeing is that there's a weird linebreak in the output now:

# C B;;
- : u =
C
 <printer print_t raised an exception: File "//toplevel//", line 1, characters 18-23: Pattern matching failed>

I couldn't figure out why though...

@tadeuzagallo That's not an issue - that's the formatter doing its job and line-wrapping!

@gasche Yes, I will (and yes, it is easy to forget, I agree!)

@dra27 That makes sense, thanks for the review and feedback!

The testsuite fails on some of our Continuous Integration machines, but I cannot reproduce the failure at home. For one machine, the content of pr7060.ml.result is as follows:

# type t = A | B
# type u = C of t
# Characters 18-54:
  let print_t out = function A -> Format.fprintf out "A";;
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
B
val print_t : Format.formatter -> t -> unit = <fun>
# Cannot find type Topdirs.printer_type_new.
# - : t = B
# - : u = C B
# 

I suppose that the issue comes from Cannot find type Topdirs.printer_type_new..

This result comes from a trunk-arm-32 machine, but trunk-linux-32, trunk-mingw-64, trunk-cygwin-64, trunk-macos-10.9, and a few others are also broken. (On the one I looked at, the .result file was the same as above.)

I suspect the issue is not caused by the patch, but the thing we are testing may not be reliable/portable enough. @dra27, do you have access to the CI infrastructure to test this? (Maybe we could skip the test on the affected configurations, or consider disabling it for now to clear the CI again.)

I do have access - I'll try to have a look later.

Hmm - I'm also seeing that with mingw64.

Is there something I can do here to help?

The fix is straightforward - I'm just trying to work out why the CI didn't pick up the problem!