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 |
| { | ||
| *pstatement = error_mark_node; | ||
| return Bvariable::error_variable (); | ||
| return LocalVariable::error_variable (); |
There was a problem hiding this comment.
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.
Probably, though I'd like to leave that for another PR
philberty
left a comment
philberty
left a comment
There was a problem hiding this comment.
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
|
I'd think it would make sense to merge this as-is, as it seems like a good stepping stone towards removing |
philberty
left a comment
philberty
left a comment
There was a problem hiding this comment.
Alright in the spirit if moving to a better memory management lets merge it.
3 participants
This should make it easier for us to move away from leaking pointers to
Bvariableeverywhere. SinceLocalVariablehas a single field of type tree, it should be the same size as a pointer toBvariable, making the switch toLocalVariablewherever possible strictly an improvement.