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 Misc.fatal_errorf #356

Merged
merged 4 commits into from Dec 23, 2015

Conversation

Projects
None yet
6 participants
@mshinwell
Copy link
Contributor

commented Dec 18, 2015

This is an extremely useful function used throughout the flambda passes.

let (_ : string) = Format.flush_str_formatter () in
Format.kfprintf (fun ppf ->
fatal_error (Format.flush_str_formatter ()))
Format.str_formatter

This comment has been minimized.

Copy link
@gasche

gasche Dec 18, 2015

Member

Flushing the string formatter and ignoring the result means that there is a risk of data loss. You should use let buf = Buffer.create 100 in ... (Format.formatter_of_buffer buf) instead.

let fatal_errorf fmt =
  let buf = Buffer.create 100 in
  Format.kfprintf
    (fun _ppf -> fatal_error (Buffer.contents buf))
    (Format.formatter_of_buffer buf)
    fmt

This comment has been minimized.

Copy link
@mshinwell

mshinwell Dec 18, 2015

Author Contributor

Does this matter given that the program is about to terminate? (Would that formatter's output actually appear if we didn't ignore that result?)

This comment has been minimized.

Copy link
@diml

diml Dec 18, 2015

Member

We need Format.kasprintf...

This comment has been minimized.

Copy link
@gasche

gasche Dec 18, 2015

Member

Does this matter given that the program is about to terminate?

Surely next year this code will be reused/duplicated to provide yet another format printer. It's fine if it is slower than it should (I did not try to share the error buffer in my implementation because the function is only called once), I'd rather avoid having it wronger than it should.

This comment has been minimized.

Copy link
@Drup

Drup Dec 22, 2015

Contributor

We need Format.kasprintf...

Fortunately, It's not terribly hard to implement, see here.

This comment has been minimized.

Copy link
@Drup

Drup Dec 22, 2015

Contributor

And I agree with @gasche, str_formatter should be avoided. Next thing you know, someone uses the compiler concurrently (like, in jsoo) and he gets mangled error messages.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2015

I tried using Gabriel's code, but it turns out to have a bug.

Jeremie suggested that to avoid other people reimplementing a tricky thing, we should just add Format.kasprintf instead, and use that. This patch now does that.

@gasche

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

Lovely patch, but why is the bootstrap modified?

(I'm curious to what the bug was -- I never tested the code but it doesn't look wrong.)

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2015

Due to the addition of the standard library.
Your code didn't flush the formatter, so nothing came out!

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2015

I mean, due to the addition to the standard library.

@gasche

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

Could you add a changelog entry? (Mark Shinwell, review by Jérémie Dimino), I guess. Then I'll merge. The changelog is important now that kasprintf is added.

@gasche

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

(I don't see why an addition to the standard library would require changing boot/ocaml{c,dep,lex}; the addition to Misc does change the linked libraries, but not does not change the observable behavior of the tools. But in fact I don't really care.)

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2015

You can't use the new function from within the compiler unless a bootstrap is performed, IIUC.
I will update the changelog.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2015

Changelog updated.

gasche added a commit that referenced this pull request Dec 23, 2015

Merge pull request #356 from mshinwell/flambda_prereq-fatal_errorf
Add Misc.fatal_errorf through Printf.kasprintf

@gasche gasche merged commit 825bc34 into ocaml:trunk Dec 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bobot

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2015

I agree with @gasche, I don't understand why adding a function to the stdlib and using it in the compiler force a bootstrap. Since I'm looking at reducing the number of case, one need to do bootstrap (eg. with #324), I would appreciate if you could precise why you think a bootstrap is needed in this case.

@Drup

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2015

I tried using Gabriel's code, but it turns out to have a bug.

What was it ? cc @dbuenzli

edit: If the answer is "Your code didn't flush the formatter, so nothing came out!" ... the code does flush the formatter.

@damiendoligez damiendoligez added this to the 4.03.0 milestone Dec 28, 2015

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.