GC.enable: raise if GC is not disabled #4945
Conversation
|
Why not do the same for |
|
@oprypin There's no assertion there, as far as I can see. The idea is that you can invoke GC.disable
run_something
GC.enableand |
src/gc/boehm.cr
Outdated
| @@ -85,6 +86,10 @@ module GC | |||
| end | |||
|
|
|||
| def self.enable | |||
| unless LibGC.is_disabled != 0 | |||
| raise "GC.enable: GC is not disabled" | |||
There was a problem hiding this comment.
We should already know we're in GC.enable from the stacktrace. We don't prefix with the method name in other parts of the stdlib at least.
There was a problem hiding this comment.
The stack trace I get is:
GC is not disabled (Exception)
0x108ef5695: *CallStack::unwind:Array(Pointer(Void)) at ??
0x108ef5631: *CallStack#initialize:Array(Pointer(Void)) at ??
0x108ef5608: *CallStack::new:CallStack at ??
0x108ee4d55: *raise<Exception>:NoReturn at ??
0x108ee4d01: *raise<String>:NoReturn at ??
0x108f126ff: *GC::enable:Nil at ??
0x108ee47f4: __crystal_main at ??
0x108ef48c8: main at ??
Seems pretty cryptic to me. The call for GC::enable is like 6 lines below.
There was a problem hiding this comment.
It's a whole another issue, but it doesn't have to be this way.
There was a problem hiding this comment.
What about "Can't enable GC: GC is not disabled". I mean, there's nothing wrong with being just a bit more verbose, right?
|
I don't have an use case, but I think it is better to not raise any exception in this case and just return. |
|
Have you read the issue? Calling |
|
Yes. I would implement it this way: def self.enable
unless LibGC.is_disabled != 0
return
end
LibGC.enable
end |
|
But if you call |
|
Doesn't closing a file twice fall under the same category of bug? I'm not against either behavior, just wanting to be consistent. |
GC.enable
puts "Hello"
GC.disableWhat do you expect that do to? To me, after the last line, the GC should be enabled. But in fact, it won't be like that, with your proposal. Closing a file twice is fine, it's an idempotent operation. Enabling a GC that's not disabled can lead to the code above, and then finding where the problem of a disabled GC is becomes super tricky. |
|
LGTM? |
Fixes #4721