-
Notifications
You must be signed in to change notification settings - Fork 180
Create LocalVariable
#3862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Create LocalVariable
#3862
Conversation
This should make it easier for us to move away from leaking pointers to Bvariable everywhere. Since LocalVariable has a single field of type tree, it should be the same size as a pointer to Bvariable, making the switch to LocalVariable wherever possible strictly an improvement. gcc/rust/ChangeLog: * backend/rust-compile-expr.cc (CompileExpr::visit): Implicitly convert LocalVariable to pointer to Bvariable. * rust-backend.h (local_variable): Return LocalVariable. (parameter_variable): Likewise. (static_chain_variable): Likewise. (temporary_variable): Likewise. * rust-gcc.cc (local_variable): Likewise. (parameter_variable): Likewise. (static_chain_variable): Likewise. (temporary_variable): Likewise. (LocalVariable::get_tree): New function. (LocalVariable::error_variable): Likewise. * rust-gcc.h (class LocalVariable): New class. Signed-off-by: Owen Avery <powerboat9.gamer@gmail.com>
I have a patch which converts |
@@ -2057,7 +2074,7 @@ temporary_variable (tree fndecl, tree bind_tree, tree type_tree, tree init_tree, | |||
|| error_operand_p (fndecl)) | |||
{ | |||
*pstatement = error_mark_node; | |||
return Bvariable::error_variable (); | |||
return LocalVariable::error_variable (); |
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.
Not a fan of error state hidden within a single class. Couldn't we return an Expected<LocalVariable>
?
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.
Probably, though I'd like to leave that for another PR
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.
Hmm not quite sure what the point of this is for. the variable magic here. The get_tree call handles this wierd case I would be almost more interested in seeing if we even need that transmute in he Bvariable get tree at all or rename Bvariable entirely to Variable or something not sure why go went with Bvariable as a naming convention but it was great for bootstrapping this frontend.
Or we should really go through the rust-gcc wrapper and get rid of all the stuf we arent using which i am pretty sure is a fair bit now there were bugs in there that was annoying to find recently like build array type
This should make it easier for us to move away from leaking pointers to
Bvariable
everywhere. SinceLocalVariable
has a single field of type tree, it should be the same size as a pointer toBvariable
, making the switch toLocalVariable
wherever possible strictly an improvement.