Fix/bytecode args write from stack#951
Conversation
84cc74b to
b3e4296
Compare
|
still needs a regression test for |
smowton
left a comment
There was a problem hiding this comment.
Broadly looks good, a few specific suggestions, plus perhaps you could factor the stack backup routines into something like backup_stack_aliases (some_predicate) where the predicate differs by instruction type, looking for field or symbol or local variable references as appropriate?
There was a problem hiding this comment.
You can use [0-9] for this
There was a problem hiding this comment.
Remove this (it will match any line)
There was a problem hiding this comment.
Probably while. I certainly try not to introduce double casts, but there are probably still places where they get introduced.
There was a problem hiding this comment.
Might as well compare the member names like the symbol comparison above?
There was a problem hiding this comment.
is done in the refactored code
tautschnig
left a comment
There was a problem hiding this comment.
Lots of mostly minor comments, but having repeated the very same comments 4-5 times makes me feel very uneasy about this. It seems a refactoring would be necessary.
There was a problem hiding this comment.
Is there any reason not to use a ranged for here?
There was a problem hiding this comment.
I refactored the code, please have a look
There was a problem hiding this comment.
As above: code_assignt assign(tmp_var, stack_entry);
There was a problem hiding this comment.
code_assignt assign(var, toassign);
There was a problem hiding this comment.
Make that code more similar to the above by introducing a temporary variable, which will then also have the source location assigned.
There was a problem hiding this comment.
code assignt assign(tmp_var, stack_entry);
b3e4296 to
ed0b3b1
Compare
ed0b3b1 to
8b4e2e9
Compare
There was a problem hiding this comment.
Use .get_component_name()
tautschnig
left a comment
There was a problem hiding this comment.
Just one minor API-usage suggestion, but otherwise looks good. Thanks a lot for all the refactoring!
concerned bytecode instructions: - iinc - pustatic - putfield - ?store - ?astore
3e52f89 to
0ce085f
Compare
|
all fixed |
alternative to #931 ; as suggested by @smowton instead of adding temporary variables for load operations, this adds temporary variables on store, and also filters for types of operations and variable names.