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

fix error handling in printf.jl #25382

Merged
merged 1 commit into from
Jan 9, 2018
Merged

fix error handling in printf.jl #25382

merged 1 commit into from
Jan 9, 2018

Conversation

KlausC
Copy link
Contributor

@KlausC KlausC commented Jan 4, 2018

Resolves the following wrong behaviour (found in v0.7 and also in v0.6):

julia> using Printf

julia> @printf "%d%d%d" 1:2...
ERROR: MethodError: no method matching ArgumentError(::String, ::String, ::Int64, ::String, ::Int64, ::String)
Closest candidates are:
  ArgumentError(::AbstractString) at boot.jl:257
  ArgumentError(::Any) at boot.jl:257
Stacktrace:
 [1] top-level scope at printf.jl:1189

After change it looks like:

julia> @sprintf "%d%d%d" 1:2...
ERROR: ArgumentError: @sprintf: wrong number of arguments (2) should be (3)

@@ -1183,7 +1187,7 @@ function _printf(macroname, io, fmt, args)
quote
G = $(esc(x))
if length(G) != $(length(sym_args))
throw(ArgumentError($macroname,": wrong number of arguments (",length(G),") should be (",$(length(sym_args)),")"))
throw(ArgumentError(string($macroname,": wrong number of arguments (",length(G),") should be (",$(length(sym_args)),")")))
Copy link
Member

@fredrikekre fredrikekre Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we don't mention the function name in the message, perhaps just change this to

throw(ArgumentError("wrong number of arguments (",length(G),") should be (",$(length(sym_args)),")"))

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same kind of error messages appears 3 times in the _printf function. Should I change them all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, perhapse its there for a reason then since both @sprintf and @printf call the same method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is called form @sprintfand @printf. But that is also visible in the stack trace. I don't see more reasons than everywhere else to put the name into the exception string.

Copy link
Contributor Author

@KlausC KlausC Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is also visible in the stack trace. <

Unfortunately not always true:

julia> @sprintf "%d" 1:2...
ERROR: ArgumentError: : wrong number of arguments (2) should be (1)
Stacktrace:
 [1] top-level scope at printf.jl:1190

julia> @sprintf "%d" 1 2
ERROR: LoadError: ArgumentError: wrong number of arguments (2) should be (1)
Stacktrace:
 [1] _printf(::String, ::Expr, ::String, ::Tuple{Int64,Int64}) at ./printf.jl:1168
 [2] @sprintf(::LineNumberNode, ::Module, ::Vararg{Any,N} where N) at /home/julia/julia/usr/share/julia/site/v0.7/Printf/src/Printf.jl:64
in expression starting at REPL[7]:1

That means, the macro name appears in the stack trace, if the exception occurs at macro expansion time, but not if it appears when the generated code is executed. That means, it would make sense to keep it in the line, I fixed, and to remove it at the other 2 spots.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets just keep it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - then it can be merged as is.

@fredrikekre fredrikekre merged commit 9b5eed2 into JuliaLang:master Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants