Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Generate and use structs to serialize requests. #439

Merged
merged 11 commits into from
May 24, 2020
Merged

Generate and use structs to serialize requests. #439

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0e5f89f
Give explicit lifetimes to non-Connection object request parameters.
khuey May 23, 2020
af24c57
Generate and use Request structs to do serialization of X requests.
khuey May 24, 2020
b0f9467
Use Self::opcode().
khuey May 24, 2020
07be126
Please clippy.
khuey May 24, 2020
86af536
Fix more clippy warnings.
khuey May 24, 2020
fd094b9
Consume `conn` in xproto serialize methods and remove allow-unused an…
khuey May 24, 2020
37385b9
Introduce some type aliases to please clippy.
khuey May 24, 2020
5272f24
Revert to the previous behavior for opcode constants.
khuey May 24, 2020
8e158fa
Fix clippy's complaints about lifetimes in send_event.
khuey May 24, 2020
d863fb6
Fix spelling.
khuey May 24, 2020
6885036
Fix clippy warning on extension trait method.
khuey May 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
581 changes: 338 additions & 243 deletions generator/src/generator/namespace.rs
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions src/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
//! | Get | `Cookie::reply` | `Cookie::reply_unchecked` |
//! | Ignore | `Cookie::discard_reply_and_errors` | Just drop the cookie |
use std::borrow::Cow;
use std::convert::{TryFrom, TryInto};
use std::io::IoSlice;

Expand All @@ -63,6 +64,7 @@ pub type SequenceNumber = u64;
pub type BufWithFds<B> = (B, Vec<RawFdContainer>);
pub type EventAndSeqNumber = (Event, SequenceNumber);
pub type RawEventAndSeqNumber<B> = (B, SequenceNumber);
pub type PiecewiseBuf<'a> = Vec<Cow<'a, [u8]>>;

/// Either a raw reply or a raw error response to an X11 request.
#[derive(Debug)]
Expand Down
44 changes: 29 additions & 15 deletions src/protocol/bigreq.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use std::io::IoSlice;
use crate::utils::RawFdContainer;
#[allow(unused_imports)]
use crate::x11_utils::{Serialize, TryParse};
use crate::connection::RequestConnection;
use crate::connection::{BufWithFds, PiecewiseBuf, RequestConnection};
#[allow(unused_imports)]
use crate::cookie::{Cookie, CookieWithFds, VoidCookie};
use crate::errors::{ConnectionError, ParseError};
Expand All @@ -34,24 +34,38 @@ pub const X11_XML_VERSION: (u32, u32) = (0, 0);

/// Opcode for the Enable request
pub const ENABLE_REQUEST: u8 = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, please do.

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

Copy link
Collaborator

@eduardosm eduardosm May 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using associated constants?

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

Copy link
Owner

@psychon psychon May 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct EnableRequest;
impl EnableRequest {
/// Serialize this request into bytes for the provided connection
fn serialize<'input, Conn>(self, conn: &Conn) -> Result<BufWithFds<PiecewiseBuf<'input>>, ConnectionError>
where
Conn: RequestConnection + ?Sized,
{
let extension_information = conn.extension_information(X11_EXTENSION_NAME)?
.ok_or(ConnectionError::UnsupportedExtension)?;
let length_so_far = 0;
let mut request0 = vec![
extension_information.major_opcode,
ENABLE_REQUEST,
0,
0,
];
let length_so_far = length_so_far + request0.len();
assert_eq!(length_so_far % 4, 0);
let length = u16::try_from(length_so_far / 4).unwrap_or(0);
request0[2..4].copy_from_slice(&length.to_ne_bytes());
Ok((vec![request0.into()], vec![]))
}
}
pub fn enable<Conn>(conn: &Conn) -> Result<Cookie<'_, Conn, EnableReply>, ConnectionError>
where
Conn: RequestConnection + ?Sized,
{
let extension_information = conn.extension_information(X11_EXTENSION_NAME)?
.ok_or(ConnectionError::UnsupportedExtension)?;
let length_so_far = 0;
let mut request0 = [
extension_information.major_opcode,
ENABLE_REQUEST,
0,
0,
];
let length_so_far = length_so_far + request0.len();
assert_eq!(length_so_far % 4, 0);
let length = u16::try_from(length_so_far / 4).unwrap_or(0);
request0[2..4].copy_from_slice(&length.to_ne_bytes());
Ok(conn.send_request_with_reply(&[IoSlice::new(&request0)], vec![])?)
let request0 = EnableRequest;
let (bytes, fds) = request0.serialize(conn)?;
let slices = bytes.iter().map(|b| IoSlice::new(&*b)).collect::<Vec<_>>();
Ok(conn.send_request_with_reply(&slices, fds)?)
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Expand Down
Loading