Generate and use structs to serialize requests.

[khuey]

This PR creates a FooRequest struct for every X11 request Foo with the appropriate parameters, and then moves the existing serialization logic out of the generated helper functions and into a method that consumes the request struct. My motivation for this is that I want to use these structs to parse X11 requests to decode protocol traffic, but the parsing code is not included in this PR. It would be straightforward to build on this to complete #212 as well (by breaking the serialize method off into its own trait and adding send_request_struct_... methods on Connection).

This is largely boilerplate. It passes all the automated tests and I've done some visual inspection of bits of the generated code as well, though obviously there's too much to go through everything line by line.

Things that I think are worth paying attention to in a review:

1. I got rid of the FOO_REQUEST constants and replaced them with a FooRequest::opcode(). I think this is cleaner but it a) does break compatibility to some extent and b) despite being a const fn opcode() cannot be used in a match arm until Tracking issue for const fn integration with pattern matching rust-lang/rust#57240 is fixed, so you may want that added back.
2. The request byte buffer can no longer live on the stack (since the function that generates it returns before we call into the connection sending machinery). It ends up getting packaged into a Vec (i.e. heap allocated) but I don't think that should be a big concern.
3. Given the above, I wanted to preserve the existing IoSlice behavior, but IoSlice cannot own buffers, so the serialization method on the struct now returns a Vec<Cow<'input, [u8]>> which are Cow::Owneds for the byte buffers generated in serialization and Cow::Borrowed for external buffers that are embedded in the struct or static data like padding.
4. SendEventRequest is a bit tricky. I've chosen for now to make its event just be an &'input [u8; 32]. I think a Cow would be better in some ways but fixed length arrays do not implement ToOwned so they don't play well with Cow, and erasing the length didn't feel right.

[psychon]

One could do both: Have a pub const and then have an opcode() method (that just returns ENABLE_REQUEST instead of 0 in this case).

Would that make sense? (I came up with this proposal when I saw the diff for multithread_test which changes a match into a series of if)

[khuey]

Yeah I think it can make sense (see my comment at the top of the thread). Want me to do it?

[psychon]

Yup, please do.

It just occurred to me: This also makes sense for symmetry with errors and events. They also have this constant.

[eduardosm]

What about using associated constants?

impl FooRequest {
    pub const OPCODE: u8 = 1;
}

[psychon]

Google finds https://doc.rust-lang.org/edition-guide/rust-2018/trait-system/associated-constants.html which says that associated constants needs Rust 1.20

Edit: Which is great, since that's less than 1.37 and so we can use that.

Follow-up question would be: Should the existing opcodes be turned into associated constants as well?

Follow-follow-up question would be: Should that be done in this PR or shall we just open an issue about it?

[eduardosm]

It just occurred to me: This also makes sense for symmetry with errors and events. They also have this constant.

If we want this, we will have to revert #391

[khuey]

I've reverted to the previous way of having a global unassociated constant.

If you wanted to do associated constants I think it makes sense to do it for requests, events, and replies all at the same time.

[psychon]

My motivation for this is that I want to use these structs to parse X11 requests to decode protocol traffic, but the parsing code is not included in this PR.

But you do want to add that in a later PR, right? What are your plans with decoding protocol traffic? Do you want to build something like xtrace?

Also, since I always wonder about something like this: This PR adds 15k lines of code, making the generated code co over 100k lines (90247 -> 106746 according to wc -l src/protocol/*)

 35 files changed, 36531 insertions(+), 19947 deletions(-)

[khuey]

My motivation for this is that I want to use these structs to parse X11 requests to decode protocol traffic, but the parsing code is not included in this PR.

But you do want to add that in a later PR, right?

Yes.

What are your plans with decoding protocol traffic? Do you want to build something like xtrace?

More or less. I have a recording of all traffic over the X socket and I want to decode it to pick out some things.

Also, since I always wonder about something like this: This PR adds 15k lines of code, making the generated code co over 100k lines (90247 -> 106746 according to wc -l src/protocol/*)

 35 files changed, 36531 insertions(+), 19947 deletions(-)

Yes, I think that's largely that each field and each request now appears on two more lines (once in the struct definition and once in the function where it copies arguments into the struct).

[eduardosm]

Looking at the clippy warnings regarding 'too complex types', I have thought of the following idea:
Have a send_request method on each *Request struct, which would behave as our previous request functions (returning a cookie), but taking parameters from self. This would avoid the need of creating a vector of Cows (and would implement #212).

Then, we can also have a 'proper' serialize function that returns a (Vec<u8>, Vec<RawFdContainer>) (although I don't know what use that function could have).

[eduardosm]

@khuey Thanks for this PR!

As you are the first developer besides me and @psychon to modify the code generator, was it easy for you to look into the generator or do you feel that there should be more explanatory comments?

[psychon]

As you are the first developer besides me and @psychon to modify the code generator, was it easy for you to look into the generator

You didn't ask me, but: I always get lost in the code generator. This was also the case with the Python version. I am always happy when I do not have to look at it. For example, the current clippy warning about unnecessary lifetimes is something that I wouldn't want to look into.

(And on the "more comments" front: My "add comments" PRs were for those parts of the code that I looked at, aka xcbgen-rs. For the actual code generator: I never read it much, I only tinkered with the places that git grep found for what I wanted to do.)

Dunno if "more comments" really help here, but I also do not feel like writing some kind of overview document that describes the general flow would help...

@khuey Thanks for this PR!

Also thanks from me. I am looking forward to your next one. :-)
Feel free to ask me to look at how to fix those clippy complaints. Even though I wrote that I do not want to. I wouldn't be sad for other suggestions on what I could work on, though... (and I am already wondering whether I should write something like xtrace / xtruss or not...)

[khuey]

@khuey Thanks for this PR!

As you are the first developer besides me and @psychon to modify the code generator, was it easy for you to look into the generator or do you feel that there should be more explanatory comments?

I felt it was fairly straightforward. I did modify the outln! macro at one point to add a comment at the end of every line with the file! and line! macros to see where code was being generated from. After that there were only a few cases where it was difficult to track down where code was coming from (mostly related to things like emit_value_serialize which don't get a line to themselves).

[khuey]

@khuey Thanks for this PR!

Also thanks from me. I am looking forward to your next one. :-)
Feel free to ask me to look at how to fix those clippy complaints. Even though I wrote that I do not want to. I wouldn't be sad for other suggestions on what I could work on, though... (and I am already wondering whether I should write something like xtrace / xtruss or not...)

If you wanted to write the intercept-and-forward an X socket part of xtrace then once I send you a PR for the request parsing stuff I think you'd have a largely complete xtrace (except for converting requests/replies/etc to terminal output).