std.net.curl: fix for -dip1000 #5052
Conversation
|
Blocking #5044 |
| @property string[string] responseHeaders() | ||
| @property string[string] responseHeaders() return | ||
| { | ||
| return p.headersIn; |
There was a problem hiding this comment.
Technically speaking, isn't this fine because you're returning a reference to GC handled memory rather than a reference to the struct itself?
There was a problem hiding this comment.
As far as the compiler is concerned, you could have used malloc to create the associative array (even though currently that's far from straightforward). The only thing that it cares about is that you're returning a reference to one of its members. scope and return are orthogonal to @nogc AFAIU.
The strange thing is that it looks like the inferred scope of this is transitive, because you're not returning a reference to a field, but a reference to the object pointed by the field. @WalterBright can you shed some light on this?
There was a problem hiding this comment.
you're not returning a reference to a field, but a reference to the object pointed by the field
Right, that's my issue with this. Because the AA isn't part of the struct and it's GC-ed this is perfectly safe.
There was a problem hiding this comment.
The root of an associative array is a pointer, and marking with return tells the compiler that the lifetime of that pointer is tied to the lifetime of the struct. The lifetime of a pointer is as long as the pointer exists, which must be a subset of how long the value contained in the pointer is live.
There was a problem hiding this comment.
But isn't the pointer to the AA not tied to the lifetime of the struct at all? The lifetime of the AA is as long as the GC doesn't collect it, right?
There was a problem hiding this comment.
This ties it to the lifetime of the struct.
There was a problem hiding this comment.
This ties it to the lifetime of the struct.
The point I'm trying to make is that there's no reason that I can see to do that, and I don't understand why we can't let the AA outlive the struct, seeing as how it will always be managed by the GC and therefore doesn't present a safety risk.
Adding return here does not make this code any safer as far as I can tell.
|
Ping @WalterBright |
There was a problem hiding this comment.
Based on what I understand of DIP1000 (which isn't much), I have to agree with @deadalnix in his comments here, that it would make much more sense if GC allocated objects had unlimited lifetimes.
Currently this is marking responseHeaders as unsafe when it's completely safe in practice.
Looks like dpl-docs doesn't support ping @CyberShadow |
|
|
Indeed all of those values are GC allocated and outlive the struct, so they shouldn't need |
|
cc @WalterBright same question as @MartinNowak |
|
Ping @WalterBright |
|
Closing as std.net.curl compiles with -di1000 now: Line 167 in 71276b2 |
8 participants
No description provided.