[stdune] add scanf module #2820
Conversation
|
This is a file produced by the external world, so this shouldn't be a code error. What about raising a user error located in p?
That would indeed be preferable. I just tried to preserve existing behaviour as much as possible. I agree we should be a lot more careful with all these exceptions.
…On Oct 30, 2019, 8:33 PM +0900, Jérémie Dimino ***@***.***>, wrote:
@diml requested changes on this pull request.
In src/dune/build.ml:
> @@ -118,7 +118,13 @@ let contents p = Contents p
let lines_of p = Lines_of p
-let strings p = Map ((fun l -> List.map l ~f:Scanf.unescaped), lines_of p)
+let strings p =
+ let f x =
+ match Scanf.unescaped x with
+ | Error () -> Code_error.raise "Build.strings" []
This is a file produced by the external world, so this shouldn't be a code error. What about raising a user error located in p?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
src/stdune/scanf.ml
Outdated
|
|
||
| let unescaped x = | ||
| match Scanf.unescaped x with | ||
| | exception End_of_file -> Error () |
There was a problem hiding this comment.
Why only these two? The doc seems to say:
Raise Scanf.Scan_failure if the input does not match the format.
Raise Failure if a conversion to a number is not possible.
Raise End_of_file if the end of input is encountered while some more characters are needed to read the current conversion specification.
Raise Invalid_argument if the format string is invalid.
(but I don't know when the other two are raised)
There was a problem hiding this comment.
I'll catch failure as well. However, I think that we shouldn't catch Invalid_argument _. A bad formatting string is just a bug.
There was a problem hiding this comment.
Of course, this does make the function a bit more dangerous to use as f could easily raise Failure for other reasons.
There was a problem hiding this comment.
@aalekseyev is away this week, I'll finish the review for him.
Of course, this does make the function a bit more dangerous to use as f could easily raise Failure for other reasons.
What about Scanf.ksscanf? It seems to be exactly for this case
There was a problem hiding this comment.
In fact, if we use ksscanf I'd say we don't even need to inspect the exception
There was a problem hiding this comment.
I pushed a commit that uses ksscanf. However, I'm unable to accomplish this without this private exception I've defined. Perhaps you can think of a way to do it without this?
There was a problem hiding this comment.
The method with a private exception looks good to me
The existing behaviour seems wrong though. Since we are touching this code, we might as well clean it up! |
rgrinberg
force-pushed
the
stdune-scanf
branch
2 times, most recently
from
8ae5880 to
171a297
Compare
rgrinberg
force-pushed
the
stdune-scanf
branch
from
171a297 to
afbc1a5
Compare
|
@diml now it's a toplevel exception |
This is a slightly safer version that only catches 2 exceptions that scanf throws Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
rgrinberg
force-pushed
the
stdune-scanf
branch
from
afbc1a5 to
38f6bdd
Compare
This is a slightly safer version that only catches 2 exceptions that
scanf throws