Ensure that Locals passed to Methods are preserved - #335
Conversation
Since both Locals and Args are stored as references internally, a plain `unwrap_reference` was causing Locals that got passed to methods to be treated as though they had been passed by reference, instead of by value. This corrects that logic, and also takes into account the strange behaviour of the Windows NT interpreter - it treats strings passed via Locals into methods as *always* passed by reference, and never by value.
| /// Returns a tuple containing: | ||
| /// - The object that should be modified | ||
| /// - A boolean indicating whether an implicit cast should occur before the store | ||
| pub fn unwrap_ref_for_store(self) -> Result<(WrappedObject, bool), AmlError> { |
There was a problem hiding this comment.
I've pulled this function out of do_store for a few reasons:
- It makes it way easier to test in unit tests
- I think it looks nicer sitting next to the other
unwrapfunctions, since it matches a large part of their functionality - It doesn't need to access any
Interpreterfields, nor protect any invariants - so it's worth moving out of thatImpl.
Counter argument could be that it's explicitly "for stores" rather than generic object behaviour. I'm sympathetic to that... but look at my shiny unit tests 😉
| // return that instead (a bit like a normal `unwrap_reference`) | ||
| // | ||
| // See issue 313 and the `store.asl` tests for more details. | ||
| let mut found_arg_to_local: Option<Result<(WrappedObject, bool), AmlError>> = None; |
There was a problem hiding this comment.
This does pollute the loop a bit. In my first draft I had a straightforward unwrap_reference and a string type check that returned before the loop. But I wasn't a fan of potentially unwrapping twice. Could change it back if you prefer - my way could be a premature optimisation.
There was a problem hiding this comment.
This is unchanged (except possibly in the comments) from the file I sent you by PM.
|
@IsaacWoods would you mind taking a look? I'm pretty confident that the logic is correct, but you might be able to think of more edge cases than I can. (Also any other interested party feel free to chip in!) |
1 participant
Since both Locals and Args are stored as references internally, a plain
unwrap_referencewas causing Locals that got passed to methods to be treated as though they had been passed by reference, instead of by value.This corrects that logic, and also takes into account the strange behaviour of the Windows NT interpreter - it treats strings passed via Locals into methods as always passed by reference, and never by value.
Fixes #313