-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
This works for Ruby code storing managed Ruby objects in global variables. |
a4aa144
to
6da1a68
Compare
This is ready for initial review. Ignore the JRuby test failure for now. Key comment: https://github.com/graalvm/sulong/pull/340/files#diff-a39e5fc5437b3cb9251c567960163ba6R36 Problem I need to solve: https://github.com/graalvm/sulong/pull/340/files#diff-ace6de25373f780bf4543873d50a681fR1051 |
This will be useful for the 30 odd global variables in the R FFI, which I have been ignoring so far (or mapping to functions assuming they are read-only) |
6da1a68
to
f28418f
Compare
} | ||
|
||
@SuppressWarnings("unused") | ||
protected boolean isUninitialized(VirtualFrame frame, LLVMGlobalVariableStorage globalVariableStorage) { |
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 if it is a good idea to have isUninitialized
, isInitialized
, isNative
and isManaged
in the LLVMNode
class, since the methods are not really related to it. I think one of the two options would work better:
- Directly call the guard, which would create a direct correspondence between the guard (e.g.,
globalVariableStorage.isInitialized
) and initialize method (e.g.,globalVariableStorage.initialize
). - Make a helper class for it, if you think that it is likely that we will replace the guard test in 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.
Fixed by putting them in a separate file and using @ImportStatic
.
Looks good! I noted a few things in inline comments. |
f28418f
to
8d57e52
Compare
No description provided.