search

alacritty / vte

Parser for virtual terminal emulators
https://docs.rs/vte/
Apache License 2.0
242 stars 56 forks source link

derive Clone for the parser struct #31

Closed doy closed 4 years ago

doy commented 4 years ago

it's a useful thing to be able to do

chrisduerr commented 4 years ago

Why? I feel like this would generally be a pretty bad idea.

doy commented 4 years ago

my tests are getting a bit unwieldy, and this seemed like a reasonable option for simplifying them (to avoid needing to manually feed in the terminal state every time). agreed that it might be confusing in other situations though, although not wrong necessarily. maybe something other than Clone might be better?

kchibisov commented 4 years ago

I wonder what's the idea behind cloning something that have a concept of state like parser, since you're inheriting state, which is not a desired behavior. Are you trying to create a bunch of the same parsers, so you're just trying to simplify creation of a bunch of similar parsers? In this case I recommend you to either create a macros for them or implement your own clone, but anyway, just wondering.

chrisduerr commented 4 years ago

@doy Could you show some examples for the tests? I'm curious as to how 'bad' it is right now and if it wouldn't be easier to just create a parser with the desired state in a function.

chrisduerr commented 4 years ago

Just as a quick update, I don't think it makes sense to bikeshed forever over such a small change. Since it's not Copy I don't think it would cause too much harm.

If you've got a testcase that looks reasonable, I think we can just merge this and get it out of the way. Though I do want to see how it would be used downstream, just so I can judge how this decision might affect other users too.

It might also be worth to derive something like PartialEq, if we want to assert that two parsers are identical.

doy commented 4 years ago

honestly, the more i think about it, i'm pretty sure i agree that it's a bad idea - while it might be mildly convenient in certain cases, it also opens up the possibility of doing things like accidentally cloning it as part of a #[derive(Clone)] of some outer struct and ending up with a nonsensical state (since i really can't think of a situation outside of a test where you could have an input stream that gets split out into two separate input streams with independent states partway through). i'm probably just going to rethink my test structure at a more fundamental level, i think - the compare_diff stuff here https://git.tozt.net/vt100-rust/tree/tests/window_contents.rs#n425 is what i initially had in mind (i currently have to pass in the full stream up to that point as the third arg to be able to recreate a parser in the correct state), but the structure of these tests is kind of a mess already, even apart from that problem.

chrisduerr commented 4 years ago

Thanks for the feedback! Let us know if you find any more convincing arguments.