Fix Issue 11945 - LockingTextWriter.put

[Poita]

The function just had sloppy constraints while assuming it was a string. This fixes it up and adds some basic unit tests.

https://d.puremagic.com/issues/show_bug.cgi?id=11945

[JakobOvrum]

It looks like this line and the two above could be better expressed as:

static if (is(Unqual!(ElementEncodingType!A) == char))

[Poita]

It also has to be a string type for the code to work. Just having a element type of const(char) is not enough (that's the cause of the bug in the first place).

[JakobOvrum]

No range should have an element type of char, they should always be dchar (e.g. std.array.front returns dchar for all strings). I suppose it's still technically possible to write, and so could erroneously appear in third-party code.

A better check could also be:

static if (is(A : const(char[])))

edit:

That said, this module is littered with bad code. In particular, issuing one write call per code point is probably terribly slow.

[Poita]

Exactly, there's nothing stopping a user from writing a range of char (an ASCII string for example), so I don't think that's a good approach.

I don't believe is(A : const(char[])) is correct either. That checks if A implicitly converts to const(char[]), so, for example, Foo below would pass the check, but ptr would not be a const(char)* and the function would fail or have unexpected behaviour.

struct Foo
{
    const(char[]) str;
    alias str this;
    @property Foo* ptr() { return &this; }
}

I believe what I have is correct.

[JakobOvrum]

Exactly, there's nothing stopping a user from writing a range of char (an ASCII string for example), so I don't think that's a good approach.

Yes, I acknowledge that it's technically possible, but as I said, it would be wrong code. In a range context, ASCII must be represented with ubyte.

I don't believe is(A : const(char[])) is correct either. That checks if A implicitly converts to const(char[]), so, for example, Foo below would pass the check, but ptr would not be a const(char)* and the function would fail or have unexpected behaviour.

Yes, AliasThis tends to subtly break generic constraints. We need a better assortment of standard traits to alleviate the need for deceptive is(T : U)-style checks.

I believe what I have is correct.

I never said it wasn't; the problem is that it's a round-about way of expressing it.

[JakobOvrum]

This PR prompted me to have a look at std.stdio, and there's a lot of awful code in there... oh well, hopefully std.io will supersede is soon.

[Poita]

Any remaining issues that need addressing?

[monarchdodra]

I think this test is useless? We already have is(ElementType!A : const(dchar). Speaking of which, isn't that const also useless? Also, style nitpick, but can we put the isInputRange test before the ElementType test in the restraints? It just seems it would make more sense that way.

[monarchdodra]

Sorry for not reviewing sooner. How about:

        {
            import std.exception : errnoEnforce;

            alias ElementEncodingType!A C;
            static assert(!is(C == void));
            static if (isSomeString!A && C.sizeof == 1)
            {
                if (orientation <= 0)
                {
                    //file.write(writeme); causes infinite recursion!!!
                    //file.rawWrite(writeme);
                    auto result =
                        .fwrite(writeme.ptr, C.sizeof, writeme.length, fps);
                    if (result != writeme.length) errnoEnforce(0);
                    return; //RETURN HERE
                }
            }
            //NO ELSE HERE

            // put each character in turn
            foreach (dchar c; writeme)
                put(c);
        }

It avoids duplicating the else branch.

Other than my style nits, the root issue seems correctly fixed to me. Address and rebase, and I'll pull.

[Poita]

Done.

[ghost]

Auto-merge toggled on

[CyberShadow]

This pull request introduced a regression:
https://d.puremagic.com/issues/show_bug.cgi?id=12375

[WalterBright]

@Poita can you please look into the regression https://issues.dlang.org/show_bug.cgi?id=12375 ?

[Poita]

Looks like it has already been fixed as per @9rnsr's comment. Apologies for introducing it!