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

Turn off POK for numbers #107

Closed
wants to merge 3 commits into from
Closed

Turn off POK for numbers #107

wants to merge 3 commits into from

Conversation

ingydotnet
Copy link
Owner

@ingydotnet ingydotnet commented May 4, 2023

Fix for #68

Using @perlpunk 's test code:

use v5.16;
use YAML::XS qw/ Load /;
use JSON::PP qw/ encode_json /;
use Devel::Peek qw/ Dump /;
my $yaml = "foo: 23";
my $data = Load $yaml;
my $json = encode_json($data);
say $json;
Dump $data->{foo};

before:

{"foo":23}
SV = PVIV(0x556868e926c8) at 0x556868e8b130
  REFCNT = 1
  FLAGS = (IOK,POK,pIOK,pPOK)
  IV = 23
  PV = 0x556869234380 "23"\0
  CUR = 2
  LEN = 10

after:

{"foo":23}
SV = PVNV(0x55a2dce90580) at 0x55a2dcead130
  REFCNT = 1
  FLAGS = (IOK,pIOK)
  IV = 23
  NV = 23
  PV = 0x55a2dd255c90 "23"\0
  CUR = 2
  LEN = 10

Note that POK is now turned off, though pIOK remains as does the PV.

Also the JSON number was not quoted on my machine.

@karenetheridge can you test if this fixes your issue?

@ingydotnet ingydotnet changed the title Turn off POK for numbers - fix #68 Turn off POK for numbers May 4, 2023
@ingydotnet ingydotnet requested a review from perlpunk May 4, 2023 09:15
@perlpunk
Copy link
Collaborator

perlpunk commented May 4, 2023

Also the JSON number was not quoted on my machine.

That's because of #68 (comment)

- Turn off interal POK flag for number scalars
Changes Outdated Show resolved Hide resolved
@karenetheridge
Copy link

🥳

@perlpunk
Copy link
Collaborator

perlpunk commented May 7, 2023

Why is this PR still open and not marked as merged?

@ingydotnet
Copy link
Owner Author

Closing.

@ingydotnet ingydotnet closed this May 10, 2023
@perlpunk
Copy link
Collaborator

For the record: merged as b6b1661

If a PR is merged manually then at least adding the commit helps for later investigation if something was actually merged or not.

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.

None yet

3 participants