Skip to content
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

AML: Suggestion - Use smart pointer instead of [u8] #173

Open
rw-vanc opened this issue Apr 6, 2023 · 2 comments
Open

AML: Suggestion - Use smart pointer instead of [u8] #173

rw-vanc opened this issue Apr 6, 2023 · 2 comments

Comments

@rw-vanc
Copy link
Contributor

rw-vanc commented Apr 6, 2023

It's very difficult to debug AML code and the AML parser due to lack of information about where an error occurs. I suggest using smart pointers instead of [u8] for the buffer type, and reporting the relative position of the cursor when an error occurs. This will allow the developer to find the offending byte much more easily.

@IsaacWoods
Copy link
Member

IsaacWoods commented Apr 16, 2023

I definitely agree this could be improved. In the past, I've got pretty far with just printing the next dozen or so bytes and seeing what isn't parsing, but it would be nicer to have more context.

I'm also guessing that this would be an iteration of the solution you've suggested in #169? If so I think doing it by tracking the offset through the slice as we go will be a more accurate solution.

I don't think we need a smart pointer, as fundamentally we will be operating on a &[u8], but I might be misunderstanding what you mean. How about replacing our current parser 'input' type (which is currently &[u8]) with a wrapper type with an offset into the overall buffer which is added to as we go through the slice?

Edit: although I've just had a look at our current parser code and I agree this looks simplest to do, because of the way we return new_input slices out of the parser, to do with pointer arithmetic under the surface. I wonder if we can have a wrapper type that wraps the entire buffer, but implements indexing by range etc. so we can keep most of our current code looking like it operates on a raw slice? This could be a fairly large upheaval, but is probably the cleanest solution in the long run.

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Apr 17, 2023

I have an implementation with this:

pub struct AmlStream<'a> {
    source: &'a [u8],
    buf: &'a [u8],
}

There was a lot of copy/paste and removal of &'s to make it work, but it seems to work fine. However, I think I will change source to usize, as it is never used except for debugging. The implementation of Debug is genuinely helpful, although I would like your thoughts on what it should look like.

The content of functions is copied into a Vec, I convert the Vec into an AmlStream at the time the function gets executed.

My IDE is complaining that I need an implementation of PartialEq for [u8; 4] and [_; 0] with AmlStream (for the test macros) but the compiler doesn't complain. I can't figure out the syntax for PartialEq<u8; N>, so if you can point me at something, that would be helpful. [Edit: cargo test is failing because of this, so fixing it is mandatory.]

I hope to submit a PR this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants