Added support for simd primitives to std.traits.Signed/Unsigned

[TurkeyMan]

I think the SIMD primitives should receive the same treatment as regular primitives in this case.
I also fixed a bug in Unsigned!float.

Didn't add a unittest, since it's not guaranteed these types will be available on the target hardware.

[ghost]

Didn't add a unittest, since it's not guaranteed these types will be available on the target hardware.

Hmm, don't we version(SIMD) or something similar to work with? Otherwise __traits(compiles) might work.

[TurkeyMan]

Even if version(SIMD), the particular type chosen for the test may not be available.
__traits(compiles) might work, didn't think of that. I've never used it before.

[TurkeyMan]

So was this a yay or a nay?

[yebblies]

static if (is(typeof(__vector(int[4])))) static assert(...); should do the trick for test cases.

[monarchdodra]

I disagree with this pull. This adds a special case in a function that was originally designed to only handle built-in integrals. What about support for big ints? What about support for a user defined fixed sized integral?

I think this should be solved using template restrictions: That's what they were designed for. First, we make Signed/Unsigned accept only integral types. From there, in the module simd, you can add your own Signed/Unsigned overloads. This way, any type can define its own behavior, without having to hook its own code into std.traits's implementation.

[TurkeyMan]

simd int vectors are builtin integrals. That's precisely why I put it here.

Maybe you're right... but I'm not convinced.

1. Signed/Unsigned are introduced in std.traits, a 'std' library. Why would these particular Signed/Unsigned implementations be promoted to core.simd? (a 'core' library, which previously had no concept of 'traits')
2. People know to expect Signed/Unsigned in std.traits. Why would anybody presume they'll find them in core.simd?
3. These are builtin types, you don't need to import core.simd to make use of them. And if they haven't imported core.simd, see point 2 for ensuing confusion.

[monarchdodra]

Ah... truth be told, I have close to 0 knowledge about SIMD. I thought they were pulled by "core.simd". Since this is not the case, my entire line of reasoning fails (as evidenced by your points).

In that case, yes, I'm fine with the implementation being in std.traits.

Let's continue this review then.

I think a pre-requisite would be for you to introduce the isSimd or isVector traits. I find it kind of strange for Signed/Unsigned to be the only function in all or std.traits to be simd aware.

I still think it would be cleaner to have:

template Unsigned (T) if (isIntegral!T) { ... }
template Unsigned (T) if (isVector!T) { ... }

But arguably, this would be a new development, so you don't have to do it if you don't want to.

In std.conv, there are analogous functions called signed/unsigned.

Other than these comments: Unittests! I have nothing to go on that your new code actually works, and no guarantee that someone else won't break it if I pull.

[TurkeyMan]

Added unit tests.

@monarchdodra If someone wants to refactor it in that way, then they're welcome to.
Your suggestion depends on a new std.traits.isVector, and I'm not sure exactly what to name that. 'vector' is an overloaded term, and I think it would be a mistake to make a function called isVector refer exclusively to a simd vector... (or maybe not? this is a separate discussion)

Personally, I like this code. It's minimally obtrusive.

[jmdavis]

Your suggestion depends on a new std.traits.isVector, and I'm not sure exactly what to name that. 'vector' is an overloaded term, and I think it would be a mistake to make a function called isVector refer exclusively to a simd vector...

One suggestion would be isSIMDVector so that it's specific.

[DmitryOlshansky]

I guess these are tabs?
Better kill em with fire before sombody else notices ;)

[ghost]

Yes, detab please.

[ghost]

@monarchdodra : Want to continue with reviewing this pull or add any more suggestions?

[TurkeyMan]

Should I add isSIMDVector and/or std.conv.signed/unsigned?

[monarchdodra]

and/or std.conv.signed/unsigned?

I think we already have that.

Should I add isSIMDVector

I'd like that.

Want to continue with reviewing this pull or add any more suggestions?

The code is mostly LGTM. It is lacking unittests though. I'd also be more comfortable with an isSIMDVector trait (public or package, I don't care).

@TurkeyMan NitPick: The phobos style guide is to place a space after an if.

[ghost]

@TurkeyMan NitPick: The phobos style guide is to place a space after an if.

The last time I saw the style guide I think it lacked that note which is why I avoided complaining about this in other pulls.

[monarchdodra]

The last time I saw the style guide I think it lacked that note which is why I avoided complaining about this in other pulls.

Now that you mention it, I can't even find the style guide specific to phobos, just a generic style guide for D code in general.

In any case, this module's current style is to do it that way, and I think the major point in phobo's style guide is to just use the existing style for existing modules.

[TurkeyMan]

I added isSIMDVector().

There's no consistency at all wrt 'if()' or 'if ()' in std.traits. It's basically chaos!
If I change that, I'm a change all of them in the file... do you really want that noise in the history?

Also, @monarchdodra, there are unittests... I'm not sure what you mean. Are they insufficient somehow? They're better than many other function in there have (none).

[TurkeyMan]

What is std.conv.signed/unsigned supposed to do in the case of overflow? Nothing? Just a dumb reinterpret cast?
If I do the std.conv functions, I'll do it as a new PR, and I'll also do to!() while I'm at it.
to!string is useful, can anyone think what what others are useful, and actually make sense for a vector?

[monarchdodra]

There's no consistency at all wrt 'if()' or 'if ()' in std.traits. It's basically chaos!
If I change that, I'm a change all of them in the file... do you really want that noise in the history?

I missed them, nevermind. Sorry.

There's no consistency at all wrt 'if()' or 'if ()' in std.traits. It's basically chaos!

Really? I counted 21 instances of if( versus 271 instances of if (.

If I change that, I'm a change all of them in the file... do you really want that noise in the history?

I'm not asking you to clean up or anything, just saying we should at least try to stick to a common convention, rather than introduce more "chaos".

What is std.conv.signed/unsigned supposed to do in the case of overflow? Nothing? Just a dumb reinterpret cast?

AFAIK, that's what they are supposed to do. We recently moved them from traits to conv, but now that you mention it, something in conv would be expected to throw on flow...

[TurkeyMan]

conv looks a lot more complex. There's a lot more work to do in that module, and things like to!string have significant knock on effects in other modules.

[TurkeyMan]

What happened to this? I want to be able to rely on it for other work...

[TurkeyMan]

Added support for fullyQualifiedName. That seemed to be the only hole in std.traits to not support __vector().

I pondered isSigned and friends for a bit, but the problem is, they seem to be used in various places to detect a signed integer type, which may then be used in typical integer operations.

[TurkeyMan]

Can anyone suggest why the autotester has failed? It works for me, and I can't reproduce the error that it's reporting :/

[ghost]

Offending code:

enum fullyQualifiedNameImplForTypes = chain!(
            format("__vector(%s[%s])", fullyQualifiedNameImplForTypes!(V, qualifiers), N)
        );

You're declaring an enum and then trying to instantiate it.

[TurkeyMan]

Oh crap... I see, I've managed to run my tests in a different phobos tree than I'm building, which are different versions, and the function was renamed since the last DMD release for some reason >_<
Man, I have no idea how I missed that.