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

fix issue 12897 - toJSON, add the escapeNonAsciiChars option #4106

Merged
merged 1 commit into from Apr 10, 2016
Merged

fix issue 12897 - toJSON, add the escapeNonAsciiChars option #4106

merged 1 commit into from Apr 10, 2016

Conversation

ghost
Copy link

@ghost ghost commented Mar 22, 2016

This new option allows a better interoperability with software made in other languages and that are not able to read string values containing non-ASCII characters.

  • no breakage
  • no additional code (excepted a test) because the encoder already existed for unicode ctrl chars

@ghost ghost changed the title fix issue 12897 - toJSON , add an option allowing to escape multi byte characters fix issue 12897 - toJSON, add an option allowing to escape multi byte characters Mar 22, 2016
{
import std.uni : isControl;

if(isControl(c))
with (JSONOptions) if (isControl(c) || ((options & escapeMBC) >= escapeMBC && c >= 0x80))
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: this line seems a little long; wrap it?

@quickfur
Copy link
Member

Other than that, LGTM.

@ghost
Copy link
Author

ghost commented Mar 25, 2016

escapeNonAsciiChars is much more accurate

@DmitryOlshansky
Copy link
Member

LGTM

@ghost ghost changed the title fix issue 12897 - toJSON, add an option allowing to escape multi byte characters fix issue 12897 - toJSON, add the escapeNonAsciiChars option Mar 28, 2016
@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

@braddr
Copy link
Member

braddr commented Apr 8, 2016

Yes, it is. Not sure why you blame the host or auto-tester rather than
dmd, a known memory hog. Something's changed and it now frequently runs
out of memory. Someone needs to spend some time diagnosing what changed
in the last few weeks and/or directly working on the memory overhead of dmd.

On 4/7/2016 4:49 PM, Basile Burg wrote:

@braddr https://github.com/braddr: you probably have noticed this too,
win-farm-1 prevents PRs to be merged since almost 24 hours with always
an OutOfMemory error. While this one initially had to be rebased it's
more clear if you look at the last column
https://auto-tester.puremagic.com/pulls.ghtml?projectid=1. Even in the
GH PR list https://github.com/D-Programming-Language/phobos/pulls
there's no green check marks anymore, which is quite abnormal.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4106 (comment)

@ghost
Copy link
Author

ghost commented Apr 8, 2016

If this only happens on a 32 bit machine that generates 64 bit code that's already a trail.

@schveiguy
Copy link
Member

@bbasile It's not a 32-bit machine, just a 32-bit compiler that generates 64-bit code. You wouldn't be able to build if it was a 32-bit machine I don't think.

@braddr I imagine what has happened is that the memory usage has crept to the threshold for dmd. I'm not sure what we can do about it, IIRC @WalterBright, the compiler never frees memory in exchange for compilation speed. It's likely we have to figure out a better way. Not completing is worse than completing slower.

@schveiguy
Copy link
Member

In addition, I think win farm 1 is the only 64-bit windows tester. So we have a big problem if this can't pass anything.

@ghost ghost closed this Apr 8, 2016
@ghost ghost reopened this Apr 8, 2016
@ghost ghost closed this Apr 9, 2016
@ghost ghost reopened this Apr 9, 2016
@ghost
Copy link
Author

ghost commented Apr 10, 2016

@braddr , can you look at https://auto-tester.puremagic.com/pull-history.ghtml?projectid=1&repoid=3&pullid=4106 ? I don't know if you've get the notification for my previous online message but it seems that there's an auto-tester bug.

  • non member PR author
  • toogled for merge by member
  • closed by author
  • reopened by author
  • still marked for merging but merge doesn't happen when all hosts have passed.

It's been like this for hours:

bug_merge

@DmitryOlshansky
Copy link
Member

Auto-merge toggled off

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

@DmitryOlshansky DmitryOlshansky merged commit b5cd354 into dlang:master Apr 10, 2016
@ghost ghost deleted the issue-12897 branch April 11, 2016 08:30
@schveiguy
Copy link
Member

@bbasile Seems the auto tester lost track of who was supposed to auto-merge it. It needs github credentials to do so.

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.

5 participants