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

adding dynamic width and precision to printf #40105

Merged
merged 7 commits into from
Feb 2, 2023

Conversation

johanmon
Copy link
Contributor

@johanmon johanmon commented Mar 19, 2021

So this is a second attempt to add dynamic width and precision to printf.

In order to handle dynamic width and precision I've added two fields to the Spec data structure. These indicate if the specification uses dynamic width and/or precision. I've also added a field to the Format data structure that tells us how many arguments a format string requires (this is no longer simply the number of specifications).

I've tried to keep as much of the existing code intact so fmt(buf, pos, arg, spec) is unchanged. We have a function fmt(buf, pos, args, argp, spec) and depending on if the specification uses dynamic width and/or precision call fmt(buf, pos, args[argp], spec). If the specification was dynamic the actual values are taken from the args and the specification is updated (we need to patch the zero flag for Ints since we now know the precision.

The plength(f, args) is handled in the same way so we first check if we have a dynamic specification and if so update it. The function now also returns the next argument position since it could be that we use one or two for dynamic values.

For the tests I've added a few explicit test but then only checked that the version with dynamic width an precision return the same string as the static versions.

Closes #40654

The previous pull request was: #33278 but since I rewrote everything and the Printf moved I thought it was better to do a new.

@fredrikekre fredrikekre requested a review from quinnj March 24, 2021 10:57
@fredrikekre fredrikekre added the display and printing Aesthetics and correctness of printed representations of objects. label Mar 24, 2021
@musm
Copy link
Contributor

musm commented May 2, 2021

Anything left to be done here or can we merge this?
Note this closes: #40654

@musm musm linked an issue May 2, 2021 that may be closed by this pull request
@johanmon johanmon requested a review from a team as a code owner July 5, 2021 09:59
@stevengj
Copy link
Member

stevengj commented Jul 5, 2021

Looks like a botched merge/rebase?

@johanmon
Copy link
Contributor Author

johanmon commented Jul 9, 2021

@stevengj Yes, complete fubar, how do I get out of this?

@stevengj
Copy link
Member

stevengj commented Jul 9, 2021

I put together a cleaned-up branch that contains all your changes to stdlib/Printf (which I think is everything in this PR?). You should be able to do:

git fetch https://github.com/stevengj/julia jmon/dynamic2
git checkout jmon/dynamic2
git push --force https://github.com/johanmon/julia jmon/dynamic2:jmon/dynamic

@giordano
Copy link
Contributor

Bump. Would be nice to eventually get this in, but the PR is still in a bad state. @johanmon are you still willing to push this through?

@stevengj
Copy link
Member

I force-pushed the rebase.

@DilumAluthge DilumAluthge removed the request for review from a team January 27, 2023 23:13
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@stevengj
Copy link
Member

Good to merge?

@johanmon
Copy link
Contributor Author

johanmon commented Feb 1, 2023

As far as I know yes.

@stevengj stevengj added the merge me PR is reviewed. Merge when all tests are passing label Feb 1, 2023
@johanmon
Copy link
Contributor Author

johanmon commented Feb 1, 2023

hmm.... just compiled everything and there is a duplication of code in Printf.jl

 rmdynamic(Printf.Spec{T}, Any, Any) where {T} 

occures twice?

@stevengj
Copy link
Member

stevengj commented Feb 1, 2023

@johanmon, good catch — looks like a manual-rebase snafu. Should be fixed now.

@giordano giordano merged commit 383f48f into JuliaLang:master Feb 2, 2023
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Feb 2, 2023
@timholy
Copy link
Member

timholy commented Jul 16, 2024

From a JET report I'm seeing this:

rmdynamic(spec::Printf.Spec{T}, args, argp) where T @ Printf ~/.julia/juliaup/julia-1.11.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.11/Printf/src/Printf.jl:297
297 function rmdynamic(spec::Printf.Spec{Val{'s'}}::Spec{T}, args::NTuple{5, String}, argp::Int64) where {T}
298     zero::Bool, width::Int64, precision::Int64 = spec::Printf.Spec{Val{'s'}}.zero::Bool, spec::Printf.Spec{Val{'s'}}.width::Int64, spec::Printf.Spec{Val{'s'}}.precision::Int64
299     if spec::Printf.Spec{Val{'s'}}.dynamic_width::Bool
300         width::String = args::NTuple{5, String}[argp::Int64]::String
301         (argp::Int64 += 1)::Int64
302     end
303     if spec::Printf.Spec{Val{'s'}}.dynamic_precision::Bool
304         precision::String = args::NTuple{5, String}[argp::Int64]::String
305         if zero::Bool && T <: Ints::Type{Union{Val{'d'}, Val{'i'}, Val{'u'}, Val{'x'}, Val{'X'}, Val{'o'}}}::Bool && precision > 0
306             zero = false
307         end
308         (argp::Int64 += 1)::Int64
309     end
310     (Spec::Type{Printf.Spec}{T}::Type{Printf.Spec{Val{'s'}}}(spec::Printf.Spec{Val{'s'}}.leftalign::Bool, spec::Printf.Spec{Val{'s'}}.plus::Bool, spec::Printf.Spec{Val{'s'}}.space::Bool, zero::Bool, spec::Printf.Spec{Val{'s'}}.hash::Bool, width::Union{Int64, String}, precision::Union{Int64, String}, false, false)::Printf.Spec{Val{'s'}}, argp::Int64)::Tuple{Printf.Spec{Val{'s'}}, Int64}
311 end

It seems to think that args::NTuple{5, String} and hence width and precision might be String. That seems problematic? Spec can only take Ints:

struct Spec{T} # T => %type => Val{'type'}
leftalign::Bool
plus::Bool
space::Bool
zero::Bool
hash::Bool
width::Int
precision::Int
dynamic_width::Bool
dynamic_precision::Bool
end

The call chain is as follows:

       Printf.Spec{Val{'s'}}(::Bool, ::Bool, ::Bool, ::Bool, ::Bool, ::String, ::Int64, ::Bool, ::Bool)
         rmdynamic(::Printf.Spec{Val{'s'}}, ::NTuple{5, String}, ::Int64)
           plength(::Printf.Spec{Val{'s'}}, ::NTuple{5, String}, ::Int64)
             computelen(::Vector{UnitRange{Int64}}, ::NTuple{5, Printf.Spec{Val{'s'}}}, ::NTuple{5, String})
               format(::IO, ::Printf.Format{Base.CodeUnits{UInt8, String}, NTuple{5, Printf.Spec{Val{'s'}}}}, ::String, ::String, ::String, ::String, ::Stri
                 nmf_skeleton!(::NMF.ProjectedALSUpd{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Int64, ::Bool, ::Float64)

nmf_skeleton! comes from NMF.jl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@printf does not accept * for passing precision or width as argument
6 participants