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

Remove write_float_fast and write_std_float_fast #149

Merged

Conversation

sim642
Copy link
Contributor

@sim642 sim642 commented Jul 18, 2022

Removes the two unused functions according to the discussion in #148 (comment).

Notably, they are in the interface, but hidden from documentation and according to sherlocode no opam package calls them. This is unlike write_float_prec and write_std_float_prec, which are used by atdgen.
I'm not sure of why the fast versions exist in the first place though. Could be considered related to #51, but that seems to concern another issue.

TODO

@mjambon
Copy link
Member

mjambon commented Jul 18, 2022

I found this in the git log:

commit 051c8feedae422fb3a802f0c049ca1bcd04590d8
Author: mjambon <mjambon@6220f132-25dc-4409-94df-f78f6c84026c>
Date:   Thu Jun 24 07:50:33 2010 +0000

    Current write_float function becomes write_float_fast, and the new
    write_float output compact numbers at the expense of speed.

I must have kept the older versions around in case someone wants to use them or see how they perform. I'm sure it's fine to get rid of them.

CHANGES.md Show resolved Hide resolved
@Leonidas-from-XIV
Copy link
Member

#148 has been merged, so this can be rebased upon master. Given @mjambon's explanations I am perfectly happy removing the code.

@sim642 sim642 force-pushed the rm-write_float_fast branch from 1d7d08f to dce2dd3 Compare July 19, 2022 10:13
@sim642 sim642 marked this pull request as ready for review July 19, 2022 10:14
Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Very nice!

@Leonidas-from-XIV Leonidas-from-XIV merged commit cc14b52 into ocaml-community:master Jul 19, 2022
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Aug 9, 2022
CHANGES:

*2022-08-09*

### Added

- Expanded documentation of exceptions (@sim642, ocaml-community/yojson#148)

### Removed

- Removed undocumented and unused functions `write_float_fast` and
  `write_std_float_fast` from `Yojson`, `Yojson.Basic` and `Yojson.Safe`
  (@sim642, ocaml-community/yojson#149)

### Fixed

- Fix out-of-bounds error occurring when parsing object field names
  with atdgen parsers using `map_ident` or `map_lexeme` (@mjambon, ocaml-community/yojson#150)
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.

3 participants