Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

safe pure nothrow sort and sum #2075

Merged
1 commit merged into from Apr 25, 2014
Merged

safe pure nothrow sort and sum #2075

1 commit merged into from Apr 25, 2014

Conversation

monarchdodra
Copy link
Collaborator

Since @9rnsr fixed 12542, it is now possible to have pure/nothrow/safe sort and sum. sum needed no work done, so I just went and made the marked unittests so as to cover the inference.

sort was a bit harder. I had to strip some formatting in error handling, so as to make the code nothrow (no big loss here I think). I also had to work a bit around 12410.

I'm not sure sort is quite as safe/pure/nothrow as it could be for every type (I haven't done too much checks), but at the very least, it is for PODs inside arrays, with predicates. That's already pretty good.

That's it.

@9rnsr: Do you know about https://issues.dlang.org/show_bug.cgi?id=12410 ?

@monarchdodra
Copy link
Collaborator Author

Review? This is pretty trivial...

@@ -9366,9 +9361,13 @@ unittest
sort(array);
assert(array == [ 1, 2, 3, 4 ]);
// sort with a delegate
bool myComp(int x, int y) { return x > y; }
bool myComp(int x, int y) @safe pure nothrow { return x > y; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these attributes already be getting inferred? If not, does this need to be marked with a bug number?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is not within a template, neither directly or indirectly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, woops, must have misread a changelog at some point. I just never noticed because I always mark mine explicitly anyways.

@JakobOvrum
Copy link
Member

LGTM. Maybe the error handling code could be ported to nothrow quite neatly by using assumeWontThrow where appropriate.

@monarchdodra
Copy link
Collaborator Author

Maybe the error handling code could be ported to nothrow quite neatly by using assumeWontThrow where appropriate.

Maybe. But even then, we'd also need to tag on assumePure, assumeSafe and assume NOGC too. Not impossible, but it is a lot of code overhead for what is just a diagnostic.

or, we could extend asserts to not be considered for inference?

@ghost
Copy link

ghost commented Apr 25, 2014

Auto-merge toggled on

ghost pushed a commit that referenced this pull request Apr 25, 2014
safe pure nothrow sort and sum
@ghost ghost merged commit 6f05fc3 into dlang:master Apr 25, 2014
@bearophile
Copy link

Perhaps I am doing something wrong, but this code doesn't compile to me, giving a "Error: 'std.algorithm.sort!("a < b", cast(SwapStrategy)0, int[]).sort' is not nothrow":

void main() nothrow {
    import std.algorithm;
    auto data = [5, 3,  2, 1];
    sort(data);
}

@monarchdodra
Copy link
Collaborator Author

Perhaps I am doing something wrong, ...

Are you correctly rebased? It's working for me.

@bearophile
Copy link

Are you correctly rebased? It's working for me.

I have updated all the dmd/Phobos/druntime files with GIT, but I still see the nothrow problem. I don't know why.

@ghost
Copy link

ghost commented Apr 26, 2014

Please rebase again, it works fine locally with commits:

DMD: dlang/dmd@c5ebee1
Druntime: dlang/druntime@6a49753
Phobos: c61c123

@bearophile
Copy link

Now it compiles and I have found the cause of my error, I was compiling using the "-debug" switch.

@ghost
Copy link

ghost commented Apr 26, 2014

Hmm if it fails with -debug then I think that's an oversight. @monarchdodra?

@monarchdodra
Copy link
Collaborator Author

Hmm if it fails with -debug then I think that's an oversight. @monarchdodra?

I'll look into it. I'd argue the root issue though is that anything in debug should be marked nothrow, and silently caught, to concede for useability. It's already pure...

https://issues.dlang.org/show_bug.cgi?id=8464
https://issues.dlang.org/show_bug.cgi?id=10854

But in the meantime, yeah, there's an issue somewhere I'll look into.

@ghost
Copy link

ghost commented Apr 26, 2014

@monarchdodra monarchdodra deleted the sumInfer branch May 4, 2014 17:45
@monarchdodra
Copy link
Collaborator Author

there's an issue somewhere I'll look into.

#2154.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants