Skip to content

Revert "support UInt & BigInt in TOML"#49576

Closed
KristofferC wants to merge 1 commit intomasterfrom
revert-47903-roger/toml-uint
Closed

Revert "support UInt & BigInt in TOML"#49576
KristofferC wants to merge 1 commit intomasterfrom
revert-47903-roger/toml-uint

Conversation

@KristofferC
Copy link
Copy Markdown
Member

@KristofferC KristofferC commented Apr 30, 2023

Reverts #47903

This caused a bunch of type instabilities in the TOML parser.

julia> code_warntype(Base.TOML.parse_datetime, Tuple{Base.TOML.Parser})
MethodInstance for Base.TOML.parse_datetime(::Base.TOML.Parser)
  from parse_datetime(l) @ Base.TOML ~/julia/base/toml_parser.jl:960
Arguments
  #self#::Core.Const(Base.TOML.parse_datetime)
  l::Base.TOML.Parser
Locals
  @_3::Int64
  ms::Int64
  s::Int64
  m::Int64
  h::Int64
  v@_8::Union{NTuple{4, Int64}, Base.TOML.ParserError}
  read_space::Bool
  day::Any
  v@_11::Any
  v@_12::Union{Bool, Base.TOML.ParserError}
  month::Any
  v@_14::Any
  v@_15::Union{Bool, Base.TOML.ParserError}
  year::Any
  v@_17::Any
Body::Any
...

@inkydragon inkydragon added the revert This reverts a previously merged PR. label May 1, 2023
@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented May 1, 2023

Can it be fixed? What is the penalty here for type instabilities? There isn't normally anything especially wrong with having that.

@fingolfin
Copy link
Copy Markdown
Member

fingolfin commented Feb 13, 2024

What is the status of this? This PR was approved in May 2023 but never merged, and now has a conflict. Do we still need the revert?

@KristofferC
Copy link
Copy Markdown
Member Author

KristofferC commented Feb 13, 2024

Let's close for now.

@vtjnash vtjnash deleted the revert-47903-roger/toml-uint branch February 13, 2024 16:27
topolarity added a commit to topolarity/julia that referenced this pull request Apr 4, 2024
@topolarity
TOML: Improve type-stability of BigInt/UInt support
1b4fc14 
From a type-stability perspective, this restores a lot of our behavior
before JuliaLang#47903. As it turns out, 10 of the 11 uses of `parse_int` introduced
in that PR are unnecessary since the TOML format already requires the
parsed value to be within a very limited range.

Note that this change does not actually revert any functionality (in
contrast to JuliaLang#49576)
@topolarity topolarity mentioned this pull request Apr 4, 2024
Merged
KristofferC pushed a commit that referenced this pull request Apr 5, 2024
@topolarity
TOML: Improve type-stability of BigInt/UInt support (#53955)
59c3c71 
From a type-stability perspective, this restores a lot of our behavior
before #47903.

As it turns out, 10 of the 11 uses of `parse_int` (now called
`parse_integer`) introduced in that PR are unnecessary since the TOML
format already requires the parsed value to be within a very limited
range.

Note that this change does not actually revert any functionality (in
contrast to #49576)
KristofferC pushed a commit that referenced this pull request Jun 13, 2024
@topolarity
TOML: Improve type-stability of BigInt/UInt support (#53955)
5bccb82 
From a type-stability perspective, this restores a lot of our behavior
before #47903.

As it turns out, 10 of the 11 uses of `parse_int` (now called
`parse_integer`) introduced in that PR are unnecessary since the TOML
format already requires the parsed value to be within a very limited
range.

Note that this change does not actually revert any functionality (in
contrast to #49576)

(cherry picked from commit 59c3c71)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@quinnj quinnj quinnj approved these changes

Assignees

No one assigned

Labels

revert This reverts a previously merged PR. TOML

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@KristofferC @vtjnash @fingolfin @quinnj @inkydragon