Code review part 1

capnproto · Aug 30, 2023 · 6f23057 · 6f23057
1 parent afdfbeb
commit 6f23057
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 38 deletions.
diff --git a/capnp/src/text.rs b/capnp/src/text.rs
@@ -24,7 +24,7 @@
 use core::fmt::{Debug, Formatter};
 use core::{convert, ops, str};
 
-use crate::{data, Error, ErrorKind, Result};
+use crate::{Error, ErrorKind, Result};
 
 #[derive(Copy, Clone)]
 pub struct Owned(());
@@ -42,7 +42,7 @@ impl crate::introspect::Introspect for Owned {
 
 #[derive(Clone, Copy, PartialEq)]
 pub struct Reader<'a> {
-    pub reader: data::Reader<'a>,
+    pub reader: &'a [u8],
 }
 
 pub fn new_reader(v: &str) -> Reader<'_> {
@@ -73,8 +73,8 @@ impl<'a> From<&'a str> for Reader<'a> {
 impl<'a> Debug for Reader<'a> {
     fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
         match self.to_str() {
-            Ok(s) => write!(f, "{}", s),
-            Err(_) => write!(f, "{:?}", self.as_bytes()),
+            Ok(s) => write!(f, "{:?}", s),
+            Err(_) => write!(f, "<invalid utf-8: {:?}>", self.as_bytes()),
         }
     }
 }

diff --git a/capnp/src/text_list.rs b/capnp/src/text_list.rs
@@ -82,51 +82,51 @@ impl<'a> IndexMove<u32, Result<crate::text::Reader<'a>>> for Reader<'a> {
 }
 
 impl<'a> Reader<'a> {
-    /// Gets the `text::Reader` as a `&str` at position `index`. Panics if `index` is
+    /// Gets the `text::Reader` at position `index`. Panics if `index` is
     /// greater than or equal to `len()`.
-    pub fn get(self, index: u32) -> Result<&'a str> {
+    pub fn get(self, index: u32) -> Result<crate::text::Reader<'a>> {
         assert!(index < self.len());
-        self.reader
-            .get_pointer_element(index)
-            .get_text(None)?
-            .to_str()
+        self.reader.get_pointer_element(index).get_text(None)
+    }
+
+    /// Gets the `text::Reader` as a `&str` at position `index`. Panics if `index` is
+    /// greater than or equal to `len()`.
+    pub fn get_as_str(self, index: u32) -> Result<&'a str> {
+        self.get(index)?.to_str()
     }
 
     /// Gets the `text::Reader` as a `&[u8]` at position `index`. Panics if `index` is
     /// greater than or equal to `len()`.
     pub fn get_as_bytes(self, index: u32) -> Result<&'a [u8]> {
-        assert!(index < self.len());
-        Ok(self
-            .reader
-            .get_pointer_element(index)
-            .get_text(None)?
-            .as_bytes())
+        Ok(self.get(index)?.as_bytes())
     }
 
-    /// Gets the `text::Reader` as a `&str` at position `index`. Returns `None` if `index`
+    /// Gets the `text::Reader` at position `index`. Returns `None` if `index`
     /// is greater than or equal to `len()`.
-    pub fn try_get(self, index: u32) -> Option<Result<&'a str>> {
+    pub fn try_get(self, index: u32) -> Option<Result<crate::text::Reader<'a>>> {
         if index < self.len() {
-            match self.reader.get_pointer_element(index).get_text(None) {
-                Ok(s) => Some(s.to_str()),
-                Err(e) => Some(Err(e)),
-            }
+            Some(self.reader.get_pointer_element(index).get_text(None))
         } else {
             None
         }
     }
 
+    /// Gets the `text::Reader` as a `&str` at position `index`. Returns `None` if `index`
+    /// is greater than or equal to `len()`.
+    pub fn try_get_as_str(self, index: u32) -> Option<Result<&'a str>> {
+        self.try_get(index).map(|r| match r {
+            Ok(s) => s.to_str(),
+            Err(e) => Err(e),
+        })
+    }
+
     /// Gets the `text::Reader` as a `&[u8]` at position `index`. Returns `None` if `index`
     /// is greater than or equal to `len()`.
     pub fn try_get_as_bytes(self, index: u32) -> Option<Result<&'a [u8]>> {
-        if index < self.len() {
-            match self.reader.get_pointer_element(index).get_text(None) {
-                Ok(s) => Some(Ok(s.as_bytes())),
-                Err(e) => Some(Err(e)),
-            }
-        } else {
-            None
-        }
+        self.try_get(index).map(|r| match r {
+            Ok(s) => Ok(s.as_bytes()),
+            Err(e) => Err(e),
+        })
     }
 }
 

diff --git a/capnpc/test/test.rs b/capnpc/test/test.rs
@@ -451,8 +451,10 @@ mod tests {
 
         let text_list = complex_list_reader.get_text_list().unwrap();
         assert_eq!(text_list.len(), 2);
-        assert_eq!(text_list.get(0).unwrap(), "garply");
-        assert_eq!(text_list.get(1).unwrap(), "foo");
+        assert_eq!(text_list.get_as_str(0).unwrap(), "garply");
+        assert_eq!(text_list.get_as_str(1).unwrap(), "foo");
+        assert_eq!(text_list.get_as_bytes(0).unwrap(), b"garply");
+        assert_eq!(text_list.get_as_bytes(1).unwrap(), b"foo");
 
         let data_list = complex_list_reader.get_data_list().unwrap();
         assert_eq!(data_list.len(), 2);
@@ -486,7 +488,7 @@ mod tests {
                 .unwrap()
                 .get(0)
                 .unwrap()
-                .get(0)
+                .get_as_str(0)
                 .unwrap()
                 == "abc"
         );
@@ -1242,9 +1244,12 @@ mod tests {
         let text_list_const = test_constants::TEXT_LIST_CONST;
         let text_list = text_list_const.get().unwrap();
         assert_eq!(text_list.len(), 3);
-        assert_eq!(text_list.get(0).unwrap(), "plugh");
-        assert_eq!(text_list.get(1).unwrap(), "xyzzy");
-        assert_eq!(text_list.get(2).unwrap(), "thud");
+        assert_eq!(text_list.get_as_str(0).unwrap(), "plugh");
+        assert_eq!(text_list.get_as_str(1).unwrap(), "xyzzy");
+        assert_eq!(text_list.get_as_str(2).unwrap(), "thud");
+        assert_eq!(text_list.get_as_bytes(0).unwrap(), b"plugh");
+        assert_eq!(text_list.get_as_bytes(1).unwrap(), b"xyzzy");
+        assert_eq!(text_list.get_as_bytes(2).unwrap(), b"thud");
 
         // TODO: DATA_LIST_CONST
 
@@ -1331,8 +1336,10 @@ mod tests {
             assert_eq!(old_version.get_old1(), 123);
             let names = old_version.get_old4().unwrap();
             assert_eq!(names.len(), 2);
-            assert_eq!(names.get(0).unwrap(), "alice");
-            assert_eq!(names.get(1).unwrap(), "bob");
+            assert_eq!(names.get_as_str(0).unwrap(), "alice");
+            assert_eq!(names.get_as_str(1).unwrap(), "bob");
+            assert_eq!(names.get_as_bytes(0).unwrap(), b"alice");
+            assert_eq!(names.get_as_bytes(1).unwrap(), b"bob");
         }
     }