fix Issue 15862 - allocating storage in pure functions should not res…#6197
fix Issue 15862 - allocating storage in pure functions should not res…#6197MartinNowak merged 2 commits intodlang:masterfrom
Conversation
|
MetaLang
left a comment
MetaLang
left a comment
There was a problem hiding this comment.
Why the weird style of assert?
| int* p1 = p(); | ||
| int* p2 = p(); | ||
|
|
||
| if (p1 is p2) assert(0); |
There was a problem hiding this comment.
To get those asserts to be ran in release.
It's trying to solve a common problem with assert: https://forum.dlang.org/thread/ed318016-16e9-965f-4d66-c0c94dd2564c@sociomantic.com
I'd say lets revisit when we fix said issue.
PetarKirov
left a comment
PetarKirov
left a comment
There was a problem hiding this comment.
The implementation looks ok (minus one small typo), and it's a step in the right direction (fixes a bug), but I'm worried that this change implies that non-@nogc memory management schemes cannot be pure (because the result of @nogc void* allocate(size_t size) could be cached/reused) which seriously limits the usability of std.experimental.allocators in pure code.
Instead of using @nogc as a proxy for determining if the result can be cached, I thought we mostly agreed to use the mutability of the return type - immutable return type -> can cache, non-immutable -> caching is not safe (e.g. caller of an() could modify the returned array, in which case using a cached result would simply yield wrong results; N.B. it doesn't matter, that an() returns null, in general the body may not be available to the compiler, and only the function signature should be relied upon).
Pinging @klickverbot as our safe-ty and purity guru :P
src/e2ir.c
Outdated
| retmethod != RETstack && | ||
| !global.params.useAssert && global.params.optimize); | ||
|
|
||
| /* Don't common subexpression functions that many allocate memory |
|
@ZombineDev: See the discussion on Bugzilla. It seems like the right choice is not to trigger referential transparency-based optimisations for return types with mutable indirections in general, rather than basing this on |
WalterBright
force-pushed
the
fix15862
branch
from
c8f6d8c to
bf85886
Compare
|
This is reimplemented to be based on the mutability of the return type. The same logic is used as for the mutability of the function parameters, so I factored that out into a nested function they both call. |
WalterBright
force-pushed
the
fix15862
branch
3 times, most recently
from
709618b to
5e66e36
Compare
…ult in caching return values of them
Current coverage is 87.35% (diff: 94.59%)@@ master #6197 diff @@
==========================================
Files 104 104
Lines 57465 57474 +9
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 50202 50209 +7
- Misses 7263 7265 +2
Partials 0 0
|
WalterBright
force-pushed
the
fix15862
branch
from
5e66e36 to
4dfc9e9
Compare
|
LGTM, @MartinNowak wanna take a look? |
|
|
||
| /* Should catch delegates and function pointers, and fold in their purity | ||
| case PUREconst: | ||
| purity = PUREconst; |
There was a problem hiding this comment.
This could increase purity again, couldn't it? How about replacing the switch with purity = min(purity, purityOfType(isref, t))?
There was a problem hiding this comment.
This could increase purity again, couldn't it?
No, because any setting of PUREweak exits the loop.
How about replacing the switch with purity = min(purity, purityOfType(isref, t))?
Because there is no reason to continue with the loop once purity is set to PUREweak.
There was a problem hiding this comment.
Too clever usage of break/continue in switch nested in a loop IMO.
Hope you agree that break Lloop is better here :).
- instead of an uncommon unconditional break at the end of the loop body
|
Auto-merge toggled on |
|
Does this mean that we can now qualify druntime defs of C alloc functions such as https://dlang.org/library/core/stdc/stdlib/malloc.html and alikes as pure? If so, I can do it! |
|
affirmative |
|
And Mallocator? |
|
affirmative |
10 participants
…ult in caching return values of them