format() returns the correct type

[vladdeSV]

Changed format() to return the same type of string that was inputted.

For instance, formatting a dstring returns a dstring and not string.

[DmitryOlshansky]

Seems sane but might break existing code. Let's wait what others have to say

[yebblies]

This makes sense to me.

[vladdeSV]

@DmitryOlshansky Out of curiosity, can you come up with a scenario where this breaks code?

[vladdeSV]

I guess using auto result = format(fmt, args) and accidentally inserting a [w|d]string and still expect a string

[DmitryOlshansky]

I guess using auto result = format(fmt, args) and accidentally inserting a [w|d]string and still expect a string

Yeah. This and maniacs using explicit types everywhere such as string for return and wstring for fmt. If it's going to hit simebody then it must be windows folks, being the most prominent use case for UTF-16.

[DmitryOlshansky]

Please squash the commits to one commit and it should be good to go

[DmitryOlshansky]

May need to whitelist @vladdeSV for auto-tester - @MartinNowak @braddr ?

[vladdeSV]

Squashed the two commits. It should be good to go?

[vladdeSV]

One thing I think should be change is to replace my template name C with something like String

Thoughts?

[9rnsr]

One thing I think should be change is to replace my template name C with something like String

C is not good for "some sting" type. I'd recommend S to you.

[vladdeSV]

I also feel S is more suitable.

[vladdeSV]

I changed the C to S

[vladdeSV]

I changed std.conv.convFormat to fit std.format.format.

However, I do not see a reason to have convFormat(), as it's simply an alias for format().

[vladdeSV]

This currently passes all tests, but I made some changes in std.algorithm.iteration where I cast a dstring to a string since it was used in an assert message @ 9097c7b

Also, I'd say that c3e86ff should simply be an alias, if not to replace the function calls in the code to std.format.format completely.

[DmitryOlshansky]

One thing I really don't like is how it uses String as template parameter. Ideally it should accept const(Char)[] and produce mutable Char[]. Then if format is pure that mutable return is implicitly convertible to immutable(Char)[] and/or const(Char)[] at call site.

This currently passes all tests, but I made some changes in std.algorithm.iteration where I cast a dstring to a string since it was used in an assert message @ 9097c7b

Using cast like that is very bad idea it just reinterprets the array, you should use to!dstring(someStr).

Exactly the kind of thing to watch out for. We are early in release cycle so it might be fine to break a few cases like that.

[DmitryOlshansky]

Do we even call it internally anywhere? It's private should be easy to just drop it...

[vladdeSV]

Yes, at a few places.
Line 1404, 1775, 3931 ...

I wonder if it would be simpler to just replace that with std.format.format

[vladdeSV]

@DmitryOlshansky

Ideally it should accept const(Char)[] and produce mutable Char[]. Then if format is pure that mutable return is implicitly convertible to immutable(Char)[] and/or const(Char)[] at call site.

I see. Yes I agree, I'll do some testing.

Using cast like that is very bad idea it just reinterprets the array, you should use to!dstring(someStr).

My bad, I did do to!string([...])

[vladdeSV]

@DmitryOlshansky I tried, I failed. I don't know how to make the function as you described.

I tried to change S to Char[], however I got errors in the manner of cannot convert char[] to string and I do now know how to make it passable. I reverted that commit.

[quickfur]

@vladdeSV Did you mark the function pure?

[quickfur]

ping @vladdeSV

[vladdeSV]

@quickfur Sorry, been busy past days. I'll try again :)

[vladdeSV]

Pong @quickfur This means I need to make formattedWrite() pure.

And a question: I built phobos with make -f win32.mak and I didn't get any errors about impure functions being called, but the auto tester does. Why?

[quickfur]

Most of Phobos is template code, so just building it may not necessarily reveal all compilation problems. Try make -f win32.mak unittest to also run unittests.

Usually that takes too long, though. I usually just run unittests for a specific module manually by doing dmd -main -unittest -of/tmp/test std/format.d && /tmp/test (or whatever the windows equivalent is).

[vladdeSV]

Changing the function to be pure needs many changes in the code. For instance, I needed to change some things in std.stdio to be pure, just to make formatted write pure.

I don't really want to fiddle around in all of these files and just put pure on things I do not know anything about. Unless I get some help I'd rather make it return a correct string type instead of char type. But having it return correct type of char array is something I feel would be very nice to have.

Piong @quickfur

[DmitryOlshansky]

Changing the function to be pure needs many changes in the code. For instance, I needed to change some things in std.stdio to be pure, just to make formatted write pure.

It's a a lot of work. Let's keep this simple and move with just changing the return type.

[vladdeSV]

@DmitryOlshansky Should I revert my latest commit?

[DmitryOlshansky]

@vladdeSV no, rather squash all of them to one

[vladdeSV]

@DmitryOlshansky Just to be clear: I should not keep the latest commit that fails the build?

[DmitryOlshansky]

@vladdeSV - kill pure, keep Char[], and make it one commit.

[vladdeSV]

@DmitryOlshansky I've squashed them to one, however just as before it does not build.

[DmitryOlshansky]

Well previously it was string aka immutable(char)[]
So just make return type immutable(Char)[], later we may safely generalize it with purity.

[vladdeSV]

@DmitryOlshansky

So just make return type immutable(Char)[]

This causes problems when return w.data; is called, namely the same Error: cannot implicitly convert expression (w.data()) of type char[] to string as we got before.

I think the current best solution is to go back to my first suggestion. I can't make it return immutable (Char)[], but I can make it return a string of the correct type.

Any other suggestions?

[DmitryOlshansky]

Char[] -- > immutable(Char)[]

[DmitryOlshansky]

I pointed specific places.

[vladdeSV]

Thank you @DmitryOlshansky !
The auto tester is doing it's stuff currently, but there should be no problems since I built it on my computer with no problems 👍

[DmitryOlshansky]

Auto-merge toggled on