Support testing static vm functions in intrinsic tests #4868
Conversation
elliottt
force-pushed
the
trevor/string-to_s-intrinsic
branch
from
a5fddae
to
a10204e
Compare
| VALUE (*sorbet_rb_str_to_s_func(void))(VALUE) { | ||
| return rb_str_to_s; | ||
| } | ||
|
|
||
| VALUE sorbet_rb_str_to_s(VALUE str) { | ||
| return rb_str_to_s(str); | ||
| } |
There was a problem hiding this comment.
Why do we have both of these? Can't we just say in the intrinsic that we expect the cached function to be rb_str_to_s?
(And then if we have that, I suppose we don't need the KnownFunction bits? At least not for this particular method...)
There was a problem hiding this comment.
rb_str_to_s is static in string.c, which is what led to all of this. The entry in the inline cache will be rb_str_to_s, not sorbet_rb_str_to_s, so there are some hoops to jump through.
There was a problem hiding this comment.
Oh, of course. Sorry, I thought these were in the codegen payload; this makes more sense now.
|
|
||
| sig {params(x: String).returns(String)} | ||
| def self.string_to_s(x) | ||
| x.to_s | ||
| end |
There was a problem hiding this comment.
Can we have some filecheck tests for this function?
| class StringSubclass < String | ||
| end |
There was a problem hiding this comment.
Shouldn't this class be defining its own to_s method to ensure that our intrinsic implementation isn't just calling rb_str_to_s directly?
There was a problem hiding this comment.
We should test both. The default implementation of String#to_s will do something different for subclasses of String, which is what this case is testing.
| // Implicit constructor from a string for backwards compability | ||
| KnownFunction(std::string name) : KnownFunction(Type::Symbol, std::move(name)) {} | ||
|
|
||
| KnownFunction(Type type, std::string name) : type{type}, name{std::move(name)} {} |
There was a problem hiding this comment.
WDYT about making this function private?
|
We have a policy of testing changes to Sorbet against Stripe's codebase before Stripe employees can see the build results here: → https://go/builds/bui_KcBOCpZjy0WhiI |
14cd06c - Extend
IREmitterHelpers::receiverFastPathTestWithCacheto support calling another function to get the address of a static vm function when emitting a comparison. This result is stored in a global variable and cached at module load time to avoid jumping back and forth between the loaded module and the vm.a10204e - As an example, this PR adds an implementation of
String#to_each, which is defined by testing the results of method search against the static symbolrb_str_to_s.c356135 - Add benchmarks for different uses of
String#to_s.Motivation
More flexible symbol based intrinsics, and a faster implementation of
String#to_s.Test plan
See included automated tests.