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

Revert "support UInt & BigInt in TOML" #49576

Closed
wants to merge 1 commit into from

Conversation

KristofferC
Copy link
Member

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
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
Member

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
Member Author

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
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)
KristofferC pushed a commit that referenced this pull request Apr 5, 2024
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
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
Labels
revert This reverts a previously merged PR. TOML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants