-
-
Notifications
You must be signed in to change notification settings - Fork 744
replace delegate with simple nested function #8530
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
Conversation
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#8530" |
|
|
||
| toValue = &toValueImpl; | ||
|
|
||
| toValue(root, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent adding a safety hole in toJSON, I suggest adding something like this outside of toValueImpl:
// Make the function infer `@system` when `json.put` is `@system`
if (0) json.put(' ');There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this improves anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely this shouldn't compile:
struct Sink {
void put(char c) @system
{
*(cast(int*) 0xDEADBEEF) = 0;
}
}
void main() @safe
{
Sink s;
auto jv = JSONValue("x");
toJson(s, jv);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A function that takes a template OutputRange cannot be invariably @trusted, that's in violation of safe interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, but I dislike doing kludges like this. At least it's not as bad as the delegate kludge being replaced.
7f9f3fc to
c14a7e0
Compare
Co-authored-by: Dennis <[email protected]>
The code has a kludge in it where it is recursively attempting to infer @safe, but that doesn't work, as the compiler assumes it is not @safe if recursive. The workaround in the code is to use a delegate. The trouble with the delegate a dynamic closure is allocated for it. Dynamic closures can escape, so dip1000 complains about
ref jsonescaping the scope.The solution is to get rid of the delegates, and use a nested function directly. Mark it as @trusted, which is also a workaround, but a better workaround than the old method, and at least this will work with dlang/dmd#14364
For the future, it would be better to simply require the
jsonparameter to be @safe.