Implement issue# 10314 (add std.traits.signed). #1341

jmdavis · 2013-06-09T09:17:08Z

This adds std.traits.signed, since for some reason, we have
std.traits.unsigned but do not have std.traits.signed.

JakobOvrum · 2013-06-09T09:23:31Z

std/traits.d

+    else
+    {
+        static assert(T.min != 0, "Bug in either signed or isIntegral");
+        return cast(Unqual!T) x;


This line is dead code. It should be removed.

edit:

Nevermind, I read wrong.

JakobOvrum · 2013-06-09T09:28:43Z

The documentation does not indicate that it removes const, immutable or shared, which it does.

Integral types are value types with no indirection, so it's not a huge issue, but considering that signed and unsigned exist primarily for generic code to begin with, I can imagine these functions being less than ideal in many situations where type inference or typeof is involved. Would it be a lot of work to make them preserve the type qualifier?

yebblies · 2013-06-09T09:29:29Z

UnsignedTypeOf?

JakobOvrum · 2013-06-09T09:34:55Z

I see that we have std.traits.Unsigned and std.traits.Signed which preserve the type qualifier. Maybe we should reuse these for the implementation of unsigned and signed?

jmdavis · 2013-06-09T09:46:18Z

On the whole, I wouldn't expect it to be a big deal to make signed and unsigned return the same level of immutability as was passed in, but with unsigned, it does technically risk breaking existing code that was using auto to hold the return value. I'm not sure that the risk is enough to not make the change, but it is something to consider.

JakobOvrum · 2013-06-09T10:04:23Z

Yeah, I understand the concern. Further, signed should behave like unsigned in this manner, so either we change unsigned as well, or implement signed with unsigned's behaviour (which this pull request currently does).

I'd argue that unsigned's current behaviour is a bug and that code that relies on it is most likely wrong already. It is not mentioned in the documentation that type qualifiers are erased. It's also inconsistent with how the similarly named Unsigned and Signed templates work, as they preserve the type qualifier.

It would be most intuitive if:

Returns the corresponding unsigned value for x

Meant immutable(int) becomes immutable(uint) and not uint.

In this interpretation, the behaviour is simply changed to match both the documentation and similar interfaces in the module.

andralex · 2013-06-09T14:00:42Z

I think we're stuck with making it remove qualifiers. @jmdavis could you please add that info to the docs with this diff?

One question that comes to mind is moving signed and unsigned to std.conv. We could put std.traits.unsigned on the deprecation path, or leave it as an undocumented alias.

monarchdodra · 2013-06-09T14:42:56Z

If we actually go through the trouble of moving them, then it would be better to also take the opportunity to give them the "correct" semantic. Simply going through the effort of moving them, yet doing only half the job seems (sorry) stupid to me.

My stance would be that the current implementation is just bugged, and should simply be fixed with no questions asked.

If we move it, then I think it should be put on a deprecation path. Having undocumented aliases in phobos doesn't help anyone. Either we move it for reals, or we don't do anything.

andralex · 2013-06-09T18:31:12Z

If we actually go through the trouble of moving them, then it would be better to also take the opportunity to give them the "correct" semantic. Simply going through the effort of moving them, yet doing only half the job seems (sorry) stupid to me.

s/stupid/odd/

There is reason, unexpectedly :o). It's one thing to have an alias to the same name in a different module; defining distinct unsigned variants, subtly different, in different modules - that's potentially more confusing.

My stance would be that the current implementation is just bugged, and should simply be fixed with no questions asked.

I don't think it's bugged. It has a spec and it implements it. People can easily add qualifiers on the receiver side. Is a qualifier-preserving definition better? Most likely. Is this a disaster? I don't think so.

If we move it, then I think it should be put on a deprecation path. Having undocumented aliases in phobos doesn't help anyone. Either we move it for reals, or we don't do anything.

OK.

monarchdodra · 2013-06-09T18:56:29Z

I don't think it's bugged. It has a spec and it implements it.

Maybe not bugged, but what it definitely has is lack of spec. I guess the implementation was free to be implemented around the hole, but at the same time, the user shouldn't rely on something not specified in the spec.

People can easily add qualifiers on the receiver side.

I think that does make a good point actually. Combining both Signed and signed should give the user the power to to anything he/she wants pretty conveniently.

auto s = signed(u); //not qualified
Signed!U s = signed(u); //same qualification
immutbale s = signed(u); //totally qualified

So with proper documentation and rationale, I think keeping it as is should work OK.

jmdavis · 2013-06-10T03:30:36Z

Okay. I updated the documentation and included an example of how to use Signed or Unsigned to retain the same level of constness.

yebblies · 2013-06-10T04:21:52Z

std/traits.d

+ */
+auto signed(T)(T x) if (isIntegral!T)
+{
+         static if (is(Unqual!T == ubyte )) return cast(byte ) x;


Can't this whole static if/else chain be replaced by return cast(SignedTypeOf!T)x;?

jmdavis · 2013-06-10T04:44:06Z

I just updated it so that it takes advantage of Signed and Unsigned.

monarchdodra · 2013-06-17T16:19:29Z

So... No use beating around the bush here I say? We either keep signed/unsigned here, and pull this, or we decide to move it to std.conv.

The fact that nobody even noticed that signed was missing would be a good indication that moving them would not be very problematic. On the other hand, given that signed does not exist yet, it would be a shame to include it here, just to deprecate it later.

Let's decide which we do... I'm in favor of putting these in std.conv.

andralex · 2013-06-17T19:30:24Z

I'd say move them and leave undocumented aliases in std.traits.

jmdavis · 2013-06-19T06:42:21Z

Okay. I moved signed and unsigned to std.conv and put an undocumented, deprecated alias for unsigned in std.traits.

monarchdodra · 2013-06-19T07:27:27Z

std/conv.d

+    (e.g from $(D int) to $(D long)).
+
+    Note that the result is always mutable even if the original type was const
+    or immutable. In order to retain the constness, use $(LREF Unsigned).


XREF now though. I think this is the correct syntax?
$(XREF traits,Unsigned)
ditto in signed

This moves std.traits.unsigned to std.conv and adds std.conv.signed, since for some reason, we have std.traits.unsigned but do not have std.traits.signed.

jmdavis · 2013-06-19T15:49:07Z

Okay, I fixed the docs.

jmdavis · 2013-06-23T02:59:54Z

Anything left on this?

monarchdodra · 2013-06-23T17:09:44Z

Anything left on this?

I have nothing to add.

LGTM.

jmdavis · 2013-07-04T05:04:15Z

Ping?

9rnsr · 2013-07-04T08:36:57Z

LGTM.

Implement issue# 10314 (add std.traits.signed).

JakobOvrum reviewed Jun 9, 2013
View reviewed changes

yebblies reviewed Jun 10, 2013
View reviewed changes

monarchdodra reviewed Jun 19, 2013
View reviewed changes

Implement issue# 10314.

378fe78

This moves std.traits.unsigned to std.conv and adds std.conv.signed, since for some reason, we have std.traits.unsigned but do not have std.traits.signed.

9rnsr added a commit that referenced this pull request Jul 4, 2013

Merge pull request #1341 from jmdavis/signed

1b230c3

Implement issue# 10314 (add std.traits.signed).

9rnsr merged commit 1b230c3 into dlang:master Jul 4, 2013

Uh oh!

Implement issue# 10314 (add std.traits.signed). #1341

Implement issue# 10314 (add std.traits.signed). #1341

Uh oh!

Conversation

jmdavis commented Jun 9, 2013

Uh oh!

JakobOvrum Jun 9, 2013

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Jun 9, 2013

Uh oh!

yebblies commented Jun 9, 2013

Uh oh!

JakobOvrum commented Jun 9, 2013

Uh oh!

jmdavis commented Jun 9, 2013

Uh oh!

JakobOvrum commented Jun 9, 2013

Uh oh!

andralex commented Jun 9, 2013

Uh oh!

monarchdodra commented Jun 9, 2013

Uh oh!

andralex commented Jun 9, 2013

Uh oh!

monarchdodra commented Jun 9, 2013

Uh oh!

jmdavis commented Jun 10, 2013

Uh oh!

yebblies Jun 10, 2013

Choose a reason for hiding this comment

Uh oh!

jmdavis commented Jun 10, 2013

Uh oh!

monarchdodra commented Jun 17, 2013

Uh oh!

andralex commented Jun 17, 2013

Uh oh!

jmdavis commented Jun 19, 2013

Uh oh!

monarchdodra Jun 19, 2013

Choose a reason for hiding this comment

Uh oh!

jmdavis commented Jun 19, 2013

Uh oh!

jmdavis commented Jun 23, 2013

Uh oh!

monarchdodra commented Jun 23, 2013

Uh oh!

jmdavis commented Jul 4, 2013

Uh oh!

9rnsr commented Jul 4, 2013

Uh oh!

Uh oh!