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 😉
There was a problem hiding this comment.
I'm sympathetic to your reasoning here. I don't see a reason an end-user would want to call this - would making it pub(crate) be reasonable?
| // 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.
I think with the above comment this is okay and probably as understandable as possible. I have no idea how NT would have ended up with this behaviour... would be interesting to see.
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!) |
|
I spotted some tests from uACPI that fail which I'd like to add, so converted back to draft for now. |
| /// 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'm sympathetic to your reasoning here. I don't see a reason an end-user would want to call this - would making it pub(crate) be reasonable?
| // 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.
I think with the above comment this is okay and probably as understandable as possible. I have no idea how NT would have ended up with this behaviour... would be interesting to see.
2 participants
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