Binary parser change #60
Conversation
|
Hey @antiagainst, I would love to hear your opinion on this, if for no other reason than that it doesn't get hopelessly out of date. |
There was a problem hiding this comment.
Sorry for dropping the ball! I like the changes to remove unsafe (num.rs) but for the decoder and parser interface, I think we need more discussion. What's the motivation for those changes?
Would you mind to separate the changes for the unsafe removal out to get it landed first?
rspirv/binary/assemble.rs
Outdated
| if bytes.len() % 4 == 0 { | ||
| words.push(0) | ||
| } | ||
| let chunks = s.as_bytes().chunks_exact(4); |
| @@ -24,8 +24,8 @@ use std::{error, fmt}; | |||
| /// Decoder Error. | |||
| #[derive(Debug, PartialEq)] | |||
| pub enum Error { | |||
| StreamExpected(usize), | |||
| LimitReached(usize), | |||
| StreamExpected, | |||
There was a problem hiding this comment.
Sounds reasonable to me.
| pub struct Parser<'c, 'd> { | ||
| decoder: decoder::Decoder<'d>, | ||
| consumer: &'c mut dyn Consumer, | ||
| pub struct Instructions<T> |
There was a problem hiding this comment.
Hmm, I'm not so sure about this. This is changing the whole parser interface but I'm not seeing how this is to be used? Another nit comment is that Parser is a more well-known term in compilers. Instruction is already heavily overloaded. Let's not make it worse.
rspirv/binary/decoder.rs
Outdated
|
|
||
| let b = f32_to_bytes(-12.34); | ||
| let mut d = Decoder::new(&b); | ||
| assert_eq!(Ok(-12.34), d.float32()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_decode_float32() { |
There was a problem hiding this comment.
So decoder is removed but we still have tests here?
| } | ||
| Ok(words) | ||
| } | ||
| pub struct Decoder { |
There was a problem hiding this comment.
I sort of like the existing decoder and parser structure because it allows other kinds of parsers. Whether there are such a need I cannot see it right now. But given it's already implemented, I think we can just keep it for now unless it becomes problematic for some feature we want to use. WDYT?
Skepfyr
force-pushed
the
parser-overhaul
branch
from
46177b7
to
08c39d6
Compare
|
Sorry, I've realised I should have explained this better. This changes the parser from being callback-based to being an Iterator of instructions, the This change was brought about by me trying to get rid of the This is a very draft-y PR most of the small changes can be ignored, I was only putting it up here for the large scale structure to be considered, and I think most of the unsafe removal came in #53. I agree this is a large change without large benefits, however seen as so much is changing right now, I feel like now is probably the best time to implement anything like this. |
|
I see your motivation better now. Thanks for the detailed information! But I feel I'm still not very convinced over this. What additional benefits does this bring except from being more Rust idiomatic? I think that's a second thing compared to designing good layering and API. (Unless the existing impl is totally off w.r.t. Rust paradigm; but that means it probably won't be able to be expressed in Rust ;-P) IMO the change proposed here blurs the layering and API boundary. I think a good API should make problematic usages harder. The previous decoder and parser is very clear regarding that. If you are using decoder, it means no understanding of the grammar, so only low-level decoding functionality and you are on your own for making sure everything works. If you are using parser, then you are only exposed to either full instruction parsing or error; no way to get a single operand of an instruction. Now with the new I guess I'm not opposed changing to make it more Rust idiomatic; I'm just worried about the public interface. As long as the API is reasonable, we can change the impl whatever way we want. WDYT? (But I may misunderstand your proposal; feel free to correct me!) |
|
I think the public API in this is more structured than it used to be (although I might not have removed some |
|
I see. So actually you'd like to make the decoder internal to the parser (which now becomes Given this is quite outdated right now (sorry I wasn't able to reply promptly!), would you mind to pocket this for now and let's focus on having SR fleshed out first? I don't want the world to be entirely broken when we cut the next release and folks suddenly find the codebase is entirely different. :) |
|
Yes, to me the decoder and parser are essentially the same, if you look at the current code the design of each are heavily intertwined. Although I do like elegant abstractions, I can't really see one here or why it would be useful, I would like to hear of any alternative use case for the decoder without the parser, I can't see one existing because in order to decode what comes next in the stream you need to have parsed what you already have. There is no reason it is harder to test because a unit testing module can access all the methods, they still exist but are private. The benefit I see of doing this is to clean up the users API, it turns from a few functions, a trait implementaton, and a whole load of methods that the user doesn't really need to touch to two functions (parse_bytes and parse_words). I'm actually happy to just leave this here, it turns out I was wrong about it getting outdated as all of the sr work doesn't touch any of this code. I'm not sure about whether it is more sensible to wait for the next release, the whole thing is essentially changing anyway it might be sensible to just get it over and done with in one go, rather than making large changes to APIs over multiple revisions. |
|
I attempted to implement some more of this just to see where it would take me. I'm now no longer convinced that this change specifically is worth it. |
I was attempting to see if I could do anything about the remaining unsafe from #53 and got slightly carried away... I thought I may as well put this up to see if you thought it would be worthwhile.
I've flipped the parser inside out so that instead of being callback-based it is an Iterator over the instructions.
It's a really large change to the API and I don't think the benefits are huge, however, it does unify a few things and is a slightly more rusty interface (IMO). It would take a bit more work to get up and running but probably only a day or two.