-
-
Notifications
You must be signed in to change notification settings - Fork 706
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
Most of std.encoding is now @safe. #4082
Conversation
Due to https://issues.dlang.org/show_bug.cgi?id=15762 , a lot of small functions are @trusted.
| } | ||
| return result; | ||
| } | ||
| alias s this; |
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.
At that point, just return a slice.
The current CodeUnits only provides foreach functionality. With this AliasThis, CodeUnits provides all functionality available to slices, including reassignment, shrinking, appending, etc.
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.
CodeUnits is part of the public interface, albeit undocumented, so I can't eliminate it in one release. I've deprecated the struct and the one function that creates it and reverted my other changes. Internally I switched std.encoding to use encode(dchar), which CodeUnits calls internally.
However, I don't know how long something should be deprecated before removal. A year? 18 months?
|
I can't find anything describing the standard library deprecation policy in either CONTRIBUTING.md or on the Wiki after a quick search. @jmdavis, maybe you could write it up somewhere persistent? |
| @@ -2250,24 +2138,27 @@ body | |||
| immutable minReservePlace = 6; | |||
| } | |||
|
|
|||
| Dst[] buffer = new Dst[s.length]; | |||
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.
Even with reserve, the old code does less work. Return values of strongly pure functions are implicitly convertible to immutable - could this be leveraged here, using a (possibly static) nested function?
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.
In order to do that, I would need to make half of std.encoding pure.
I can restore the old code as @trusted as an interim step, or I can make everything pure and we can come back to this in a week or two.
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.
The performance difference could be negligible for all I know, I haven't measured it. I would have thought most of std.encoding was pure already, but if it's extra work then I think it's better to leave it to the future.
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.
Compared to idup, it allocates less, but it switches from a single array copy to a series of small array copies. This is a more predictable cost, though not necessarily a smaller one.
Compared to the previous code, it's a series of small slice copies. Shouldn't be terribly expensive.
Most of std.encoding would be pure, but it's using an interesting (and moderately annoying) system to minimize the amount of duplicate code. That system obscures which cases can be pure. Switching functions to no-arg template functions will make some things just work, but encoding to an array, for instance, will take some redesign. And encoding to an array is the case we need here.
| @@ -652,7 +662,8 @@ template EncoderInstance(E) | |||
|
|
|||
| private template GenericEncoder() | |||
| { | |||
| bool canEncode(dchar c) | |||
| @safe: | |||
| bool canEncode(dchar c) @safe | |||
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.
Is the @safe here necessary? Since this is a template member, shouldn't the compiler already infer attributes for it?
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.
It could prevent regressions of this fix.
|
Needs rebase |
|
If the cast between the two arrays is unconditionally safe, can't you just add a small, private |
|
ping @dhasenan |
+1 - it's also possible to define @trusted lambda's and directly execute them ;-) |
|
ping @dhasenan |
|
Looks like this has been overtaken by events and someone else did most of what I did in a slightly different way. Closing this; will reopen if necessary. |
Due to https://issues.dlang.org/show_bug.cgi?id=15762 , a lot of small functions are @trusted. Specifically, std.encoding uses an 'enum : ubyte' pattern, and casts between const enum arrays and value types are not allowed in @safe code for no discernible reason.
Several templates are not marked @safe or @trusted so @safe can be inferred as appropriate.