Add SignatureTranslatorPrism and TypeParamsToParserNodesPrism #815
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
allcre
force-pushed
the
ac-add_signaturetranslatorprism_and_typeparamstoparsernodesprism
branch
from
376f9e6 to
3a61f26
Compare
allcre
force-pushed
the
ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation
branch
from
8914ab9 to
77219b7
Compare
allcre
force-pushed
the
ac-add_signaturetranslatorprism_and_typeparamstoparsernodesprism
branch
from
3a61f26 to
ebf3230
Compare
allcre
force-pushed
the
ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation
branch
from
77219b7 to
005a338
Compare
| rbs_string_t rbsString = makeRBSString(assertion.string); | ||
| const rbs_encoding_t *encoding = RBS_ENCODING_UTF_8_ENTRY; | ||
|
|
||
| Parser parser(rbsString, encoding); |
There was a problem hiding this comment.
Uniform initialization
| Parser parser(rbsString, encoding); | |
| Parser parser{rbsString, encoding}; |
| rbs_string_t rbsString = makeRBSString(declaration.string); | ||
| const rbs_encoding_t *encoding = RBS_ENCODING_UTF_8_ENTRY; | ||
|
|
||
| Parser parser(rbsString, encoding); |
There was a problem hiding this comment.
This repeats 5 times, can you extract a private helper?
| auto nameStr = parser.resolveConstant(rbsTypeParam->name); | ||
| auto nameConstant = ctx.state.enterNameConstant(nameStr); | ||
|
|
||
| vector<pm_node_t *> args; |
There was a problem hiding this comment.
This will only have 0 or 1 elements. We can skip this heap allocation if we use an inlined vector on the stack:
| vector<pm_node_t *> args; | |
| absl::InlinedVector<pm_node_t *, 1> args; |
| pm_node_t *block = nullptr; | ||
| if (defaultType || upperBound || lowerBound) { | ||
| auto typeTranslator = TypeToParserNodePrism(ctx, {}, parser, prismParser); | ||
| vector<pm_node_t *> pairs; |
There was a problem hiding this comment.
Similar
| vector<pm_node_t *> pairs; | |
| absl::InlinedVector<pm_node_t *, 3> pairs; |
|
|
||
| private: | ||
| core::MutableContext ctx; | ||
| parser::Prism::Parser *parser; // For Prism node creation |
There was a problem hiding this comment.
Passed to a few translators:
auto methodTypeToParserNodePrism = MethodTypeToParserNodePrism(ctx, move(parser), *this->parser);
There was a problem hiding this comment.
Those are different, there's Parser parser local variables that shadow this->parser
There was a problem hiding this comment.
So if I'm understanding correctly the 2nd arg in the line of code above is the local variable parser but the 3rd arg is the member variable? So it is being used? I can also rename all the local variables to rbsParser to avoid this confusion.
There was a problem hiding this comment.
Yeah, in the context where we have both, it would be more clear to give one or both of them prefixes. Relying on variable shadowing to draw the distinction is pretty non-obvious
allcre
force-pushed
the
ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation
branch
from
005a338 to
4536485
Compare
allcre
force-pushed
the
ac-add_signaturetranslatorprism_and_typeparamstoparsernodesprism
branch
3 times, most recently
from
284ba10 to
820e412
Compare
Co-authored-by: Kaan Ozkan <kaan.ozkan@shopify.com>
allcre
force-pushed
the
ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation
branch
from
4536485 to
388f9ab
Compare
allcre
force-pushed
the
ac-add_signaturetranslatorprism_and_typeparamstoparsernodesprism
branch
from
82b09ef to
58b03c9
Compare
2 participants
Motivation
Test plan
See included automated tests.