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 15884 Encode wstring/wchar[]/dstring/dchar[] as JSON string #4185

Merged
merged 1 commit into from Apr 17, 2016

Conversation

lionello
Copy link
Contributor

This fix will now encode all string types, mutable and immutble, as JSON strings:

    wchar[] wstr = "a"w.dup;
    JSONValue testValw = wstr;
    assert(testValw.type == JSON_TYPE.STRING);

https://issues.dlang.org/show_bug.cgi?id=15884

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
15884 Assigning char[] to std.json.JSONValue creates array, not string

@@ -346,10 +346,11 @@ struct JSONValue
type_tag = JSON_TYPE.STRING;
store.str = arg;
}
else static if(is(T : char[]))
else static if(isNarrowString!T)
Copy link
Member

Choose a reason for hiding this comment

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

You need to use isSomeString, because isNarrowString would not allow dchar-based strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isSomeString might not be good either: "Static arrays of characters (like char[80]) are not considered built-in string types."

@lionello lionello force-pushed the fix15884 branch 2 times, most recently from af269d5 to 02e2a02 Compare April 12, 2016 05:06
type_tag = JSON_TYPE.STRING;
store.str = arg.idup;
store.str = byUTF!char(arg).array.dup;
Copy link
Member

Choose a reason for hiding this comment

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

No need for .dup, .array already does a copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know. But wouldn't work without it.

Even though the result from "array" is unique, I can't seem to make it "immutable" in safe code. Please help?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't saw your comment. I'll try to find what the issue is.

@@ -1700,7 +1701,10 @@ pure nothrow @safe @nogc unittest

pure nothrow @safe unittest // issue 15884
Copy link
Member

Choose a reason for hiding this comment

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

How about:

pure nothrow @safe
unittest // issue 15884
{
    import std.meta : AliasSeq;
    import std.traits : MutableOf, ConstOf, ImmutableOf;

    T[3] staticArr(T)() { return ['a', 's', 'd']; }
    T[] dynamicArr(T)() { return ['a', 's', 'd']; }

    foreach (Char; AliasSeq!(char, wchar, dchar))
        foreach (Qualifier; AliasSeq!(MutableOf, ConstOf, ImmutableOf))
            foreach (TestString; AliasSeq!(staticArr, dynamicArr))
            {
                // From lvalue:
                {
                    auto str = TestString!(Qualifier!Char);

                    // test construction
                    JSONValue testVal = str;
                    assert(testVal.type == JSON_TYPE.STRING);

                    // test assignment
                    testVal = str;
                    assert(testVal.type == JSON_TYPE.STRING);
                }

                // From rvalue:
                {               
                    // test construction
                    JSONValue testVal2 = TestString!(Qualifier!Char);
                    assert(testVal2.type == JSON_TYPE.STRING);

                    // test assignment
                    testVal2 = TestString!(Qualifier!Char);
                    assert(testVal2.type == JSON_TYPE.STRING);
                }
            }
}

Copy link
Member

Choose a reason for hiding this comment

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

The above will test the following types:

char[3] char[] const(char[3]) const(char)[] immutable(char[3]) string
wchar[3] wchar[] const(wchar[3]) const(wchar)[] immutable(wchar[3]) wstring
dchar[3] dchar[] const(dchar[3]) const(dchar)[] immutable(dchar[3]) dstring

Copy link
Member

Choose a reason for hiding this comment

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

If you want to add support for static arrays, you will need fix JSONValue.this, because otherwise the assignment from rvalue static array in the above test won't compile.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, this string type testing pattern can be further generalized:

pure nothrow @safe
unittest // issue 15884
{   
    import std.meta : AliasSeq;
    import std.traits : MutableOf, ConstOf, ImmutableOf;

    foreach (Char; AliasSeq!(char, wchar, dchar))
        foreach (Qualifier; AliasSeq!(MutableOf, ConstOf, ImmutableOf))
            foreach (testStr; TestString!(Qualifier!Char).strings)
            {
                // test construction
                JSONValue testVal = testStr();
                assert(testVal.type == JSON_TYPE.STRING);                   

                // test assignment
                testVal = testStr();
                assert(testVal.type == JSON_TYPE.STRING);
            }
}

version (unittest)
private struct TestString(T)
{
    import std.typecons : tuple;
    enum strings = tuple(&this.init.staticArrLvl, &this.init.dynamicArrLvl,
                         &this.init.staticArrRvl, &this.init.dynamicArrRvl);

    pure nothrow @safe @nogc:

    T[3] s = ['a', 's', 'd'];
    T[] d = ['a', 's', 'd'];
    ref T[3] staticArrLvl()  { return s; }
    ref T[]  dynamicArrLvl() { return d; }
        T[3] staticArrRvl()  { return s; }
        T[]  dynamicArrRvl() { return d; }
}

@lionello
Copy link
Contributor Author

For now, I've considered static arrays out of scope for bug 15884 so let's just get the simple cases done. It's easy to convert a static (or any) array to a dynamic one by just adding a pair of [].

@lionello
Copy link
Contributor Author

@DmitryOlshansky I saw you had merged PR#4176 from @bbasile before. This is an extension of that. Please check.

@DmitryOlshansky
Copy link
Member

LGTM

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

@DmitryOlshansky DmitryOlshansky merged commit 1f37557 into dlang:master Apr 17, 2016
@lionello lionello deleted the fix15884 branch May 30, 2016 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants