Make parseUUID pure

[monarchdodra]

http://dlang.org/phobos/std_uuid.html#parseUUID :

BUGS:
Could be pure, but this depends on parse!(string, 16).

I was reading this, and I remembered that recently, parse!(string, 16) was made pure (and safe), so I changed the code accordingly. It now only relies on inference (and a @trusted exception builder).

Tagging @jpf91 .

[jpf91]

LGTM.
I guess this constructor:
http://dlang.org/phobos/std_uuid.html#UUID%28string%29
should now work with pure as well?

[monarchdodra]

I guess this constructor: should now work with pure as well?

I missed that. I only did that one function, but I should really check the entire module. I'll do it ASAP (a day or two).

[monarchdodra]

Alright, I ended giving the entire module a minor facelift:

• Removed dependency on std.algorithm (I think a we can also remove a lot more).
• Removed not-needed anymore "parameterless templates" for template/non-template overloads (eg this()(int a)).
• Changed parameter passing to use "in". This allows initializing an UUid from an immutable dynamic array.
• Used "documented unittests".

This is supposed to be "trivial" changes only, so if you see anything you don't agree with, I'll just straight up remove it.

[monarchdodra]

I removed this comment, since "complexity" makes no sense in the context of a UUID, which has constant length.

[jpf91]

You're of course right. I have to admit back then I didn't really understand the complexity concept. Probably shows that I don't have a formal background in computer science ;-)

[jmdavis]

Technically, it is linear complexity. It's just that n is always a fixed value, so there isn't much point to the comment.

[jpf91]

Those changes look good to me except for the change removing @trusted. See inline comment for details.

BTW: Thanks for doing this, this would probably have been my job otherwise ;-)

[monarchdodra]

Alright, randomUUID is now trusted again.

[monarchdodra]

Alright, randomUUID is now trusted again.

Just be clear, it basically means I left that part "as-is" (which may or may not be correct, but that is outside of the bounds of this PR now). Are there any other changes anybody sees as problematic?

[jmdavis]

I'm not sure that it's worth rejecting this over, but I honestly think that we should never use in. The fact that it mixes in scope - which is unimplemented in the vast majority of cases - is just plain dangerous. We run a serious risk of code breakage if/when scope is properly, fully implemented if in is ever used where scope wouldn't work. And honestly, if it were up to me, in would be removed from the language. Combining two attributes in one like that just invites misuse, particularly when so many people seem to like to use it as the opposite of out without taking into account what it actually means.

[JakobOvrum]

Any breakage caused by scope being implemented/fleshed out will have found (potentially very serious) bugs.

[monarchdodra]

Actually, in this case, I chose in specifically because it meant both const and scope. scope don't make much difference here, since it's a value type anyways, but in the later sig, you have in T[] uuid: that's a reference type, so scope would make more sense here. Also, when passed by ref, the reference never escapes scope. So I believe "in" is correct.

That said, I noticed you can't use scope (or const scope) and ref together, yet you can pass by in ref. since in is supposed to be const scope, so I'm a bit confused.

Please advise what you think is actually correct here.

[jmdavis]

Part of the problem here is that what scope means is not well-defined. All the spec says on the matter is "references in the parameter cannot be escaped (e.g. assigned to a global variable)". I don't believe that it even talks about scope affecting closures. You get the basic idea of what scope is supposed to do but not the specifics. And based on what TDPL lists in its index, as far as I can tell, it doesn't discuss it either.

The only thing that I am aware of that is fully agreed upon that scope is supposed to do (and the only thing that is even partially implemented for it) is to make it so that a closure is not allocated when a delegate function parameter is marked with it. And I don't believe that even that is fully implemented.

There are plenty of folks who assume that scope should impact ref or arrays or your ability to take the address of a parameter or something that it points to, etc. But AFAIK, there is no official word or decision on any of that. And given Walter's aversion to flow analysis, I don't know how we could ever implement scope properly even for delegates, let alone anything else. As far as I can tell, fully implementing scope (especially if it involved all types rather than just delegates) either requires fairly advanced flow analysis or makes it so that you can do almost nothing with the parameter, because the compiler can't guarantee that you're not doing something which makes it or anything it refers to escape the current scope.

So, honestly, I'm inclined to argue that the only place that scope should be used at this point is delegate parameters, and that in should be used absolutely nowhere. We don't even know for sure what they're supposed to do, and if/when they're finally implemented, there's a high chance that we'll have used them incorrectly, so using them just increases the odds of breaking code later.

[JakobOvrum]

That said, I noticed you can't use scope (or const scope) and ref together, yet you can pass by in ref. since in is supposed to be const scope, so I'm a bit confused.

I think that's just part of the quirkiness of the implementation. i.e. it's a bug. I think there are also other parameter storage class combinations which should work but are rejected by the compiler as an invalid combination.

[JakobOvrum]

I don't believe that it even talks about scope affecting closures.

It doesn't need to because it's purely an optimization.

We don't even know for sure what they're supposed to do, and if/when they're finally implemented, there's a high chance that we'll have used them incorrectly, so using them just increases the odds of breaking code later.

It says right there in the spec what it's supposed to do. When we see scope used in pull requests, we should verify that it's used correctly (that it doesn't escape the parameter, to the strictest degree, i.e. including indirections). Then if it breaks code when implemented, then that code was broken anyway, because we knew we intended the parameter to not be escaped.

The only good argument against using scope is if we suppose that it might get removed, but even then it's a trivial search & replace to fix broken code.

[monarchdodra]

So, should I just mark these as const? It's the "least involved change"...

[MartinNowak]

What's the state of this?

[monarchdodra]

What's the state of this?

AFAICT, it's waiting for a merge.

[MartinNowak]

LGTM

[monarchdodra]

Thanks

[MartinNowak]

General remark, I'm somewhat biased regarding the use of templated functions to trigger attribute inference.
At least on the API layer attributes should be explicit.
The problem that we require too much annotations for internal functions should be handled by making the compiler smarter.

[monarchdodra]

General remark, I'm somewhat biased regarding the use of templated functions to trigger attribute inference.
At least on the API layer attributes should be explicit.
The problem that we require too much annotations for internal functions should be handled by making the compiler smarter.

I'd agree, but in this case, it does allow correct inference, whereas it used to be un-conditionally marked as trusted, which, IMO, is even worst. I had to chose the least of two evils on that one.

[dnadlinger]

Seems like this introduced a @safe-ty violation: https://issues.dlang.org/show_bug.cgi?id=14135

[monarchdodra]

Indeed, but the code does contain a trusted "binary trusted copy". Gonna see if I can't fix this.