Owning reference to a string with O(1) access to lines #3

pacak · 2022-03-03T04:18:49Z

This can be useful if you have a string that you don't want to modify but do want to access line by line in a random order.

pacak · 2023-06-12T21:09:32Z

Any thoughts?

vallentin · 2023-06-12T23:49:11Z

Hey. Sorry. I completely overlooked this PR. I just randomly did some changes today, and well, your new comment pinged me. I'll just take a look at this PR :)

vallentin · 2023-06-12T23:54:22Z

I have a few questions and thoughts. So feel free to respond to those, before you spend time reworking the PR.

(I'll review the PR now, and add the questions and thoughts as comments)

vallentin

Again. Sorry for the lack or response until now. I somehow overlooked the original notification for this PR

vallentin · 2023-06-12T23:55:34Z

src/lib.rs

+/// - [`with_ending`](CachedLines::with_ending)
+///
+///
+#[cfg(feature = "alloc")]


Might be easier to avoid needing all the #[cfg(feature = "alloc")], by having a:

#[cfg(feature = "alloc")] mod cached;

and then adding this implementation to a cached.rs module.

I'm okay either way, was mostly trying to keep things in one file, second alternative is

mod cached { ... }

Will do.

vallentin · 2023-06-12T23:59:41Z

src/lib.rs

+#[cfg(feature = "alloc")]
+pub struct CachedLines {
+    content: alloc::string::String,
+    splits: alloc::vec::Vec<Range<usize>>,


Maybe instead we could have a:

pub struct LineSpanWithoutText { start: usize, end: usize, ending: usize, }

and then:

Suggested change

splits: alloc::vec::Vec<Range<usize>>,

splits: alloc::vec::Vec<LineSpanWithoutText>,

That way we could replace with_ending() and without_ending() with new()

It's been a while but as far as I remember the reasoning was that you are very unlikely to use both representations at once and having just one representation gives Index and iterator goodies.

vallentin · 2023-06-13T00:01:34Z

src/lib.rs

+    pub fn get(&self, index: usize) -> Option<&str> {
+        self.content.get(self.splits.get(index)?.clone())


(in extension of the previous comment)

Then have this be something like:

Suggested change

pub fn get(&self, index: usize) -> Option<&str> {

self.content.get(self.splits.get(index)?.clone())

pub fn get(&self, index: usize) -> Option<LineSpan<'_>> {

// i.e. from splits get LineSpanWithoutText + self.contents => LineSpan

...

Yea, for get this works.

vallentin · 2023-06-13T00:03:03Z

src/lib.rs

+    type Output = str;
+
+    fn index(&self, index: usize) -> &Self::Output {
+        &self.content[self.splits[index].clone()]


Hmm. I was about to suggest calling .get() and returning the LineSpan, but this needs to return a reference

Index needs to return a reference to so to accommodate both with and without ending - it needs to be decided during construction. Either two different constructors - like I did or using either newtype/type parameter.

vallentin · 2023-06-13T00:04:15Z

src/lib.rs

 #[cfg(test)]
 mod tests {
    use super::*;

+    #[test]
+    fn test_cached_lines_no_endings() {
+        let text = "\r\nfoo\nbar\r\nbaz\nqux\r\n\r";


Not that Rust 1.70 changed the behavior, so trailing \r will appear in the final line. (Just in case this test suddenly)

PR: rust-lang/rust#100311

vallentin · 2023-06-13T00:05:41Z

src/lib.rs

+pub struct CachedLines {
+    content: alloc::string::String,


Maybe this can use alloc::borrow::Cow instead to allow for borrowing a &str instead of needing to turn it into an owned String

(Not sure if it creates other issues)

Suggested change

pub struct CachedLines {

content: alloc::string::String,

pub struct CachedLines<'a> {

content: alloc::borrow::Cow<'a>,

I'm okay either way, but Cow adds complexity without adding significant benefits - CachedLines will have to allocate a vector of sizes regardless, adding a single allocation seems to be fine compared to the alternative of allocating and storing a whole bunch of lines.

vallentin · 2023-06-13T00:06:31Z

src/lib.rs

+///
+///
+#[cfg(feature = "alloc")]
+pub struct CachedLines {


Finally, maybe it should be called LineSpanCache/CachedLineSpans. If it's changed to collect LineSpan

Names are hard, sure.

vallentin · 2023-06-13T00:09:42Z

src/lib.rs

+#[derive(Debug, Clone)]
+/// Owning pointer to a string that provides O(1) access to line slices
+///
+/// ```
+/// use line_span::CachedLines;
+/// let cache = CachedLines::without_ending(String::from("hello\nworld"));
+/// assert_eq!(&cache[0], "hello");
+/// assert_eq!(&cache[1], "world");
+/// ```
+///
+/// Slices might or might not include line breaks depending on a function used to construct it
+///
+/// - [`without_ending`](CachedLines::without_ending)
+/// - [`with_ending`](CachedLines::with_ending)
+///
+///
+#[cfg(feature = "alloc")]
+pub struct CachedLines {


Please move the attribute, to after the comment

#[derive(Clone, Debug)] pub struct CachedLines {

vallentin · 2023-06-13T00:10:34Z

src/lib.rs

+#[derive(Debug, Clone)]
+/// Owning pointer to a string that provides O(1) access to line slices
+///
+/// ```
+/// use line_span::CachedLines;
+/// let cache = CachedLines::without_ending(String::from("hello\nworld"));
+/// assert_eq!(&cache[0], "hello");
+/// assert_eq!(&cache[1], "world");
+/// ```
+///
+/// Slices might or might not include line breaks depending on a function used to construct it
+///
+/// - [`without_ending`](CachedLines::without_ending)
+/// - [`with_ending`](CachedLines::with_ending)
+///
+///
+#[cfg(feature = "alloc")]
+pub struct CachedLines {


I also feel like this should probably be feature gated, with a cache = [] feature

pacak

Currently I inlined proposed changes into my crate and using Index to access strings, code is here: https://github.com/pacak/cargo-show-asm/blob/master/src/cached_lines.rs

There's no rush to get this included :)

Open questions are:

Cow - yes/now. I decided against it since it's complexity without significant gains
stored values - Range<usize> or Linespan - former offers uniform interface for get and Index, later seems to be a bit less useful
put under separate feature or not. I decided against it, "alloc" limits what platforms you can run on, having "cached" as a separate feature saves a tiny bit of compilation time at expense of user confusion :)

What are your preferences herE?

pacak · 2023-06-13T00:12:02Z

src/lib.rs

+/// - [`with_ending`](CachedLines::with_ending)
+///
+///
+#[cfg(feature = "alloc")]


I'm okay either way, was mostly trying to keep things in one file, second alternative is

mod cached { ... }

Will do.

pacak · 2023-06-13T00:13:21Z

src/lib.rs

+///
+///
+#[cfg(feature = "alloc")]
+pub struct CachedLines {


Names are hard, sure.

pacak · 2023-06-13T00:39:12Z

src/lib.rs

+pub struct CachedLines {
+    content: alloc::string::String,


I'm okay either way, but Cow adds complexity without adding significant benefits - CachedLines will have to allocate a vector of sizes regardless, adding a single allocation seems to be fine compared to the alternative of allocating and storing a whole bunch of lines.

pacak · 2023-06-13T00:56:14Z

src/lib.rs

+#[cfg(feature = "alloc")]
+pub struct CachedLines {
+    content: alloc::string::String,
+    splits: alloc::vec::Vec<Range<usize>>,


It's been a while but as far as I remember the reasoning was that you are very unlikely to use both representations at once and having just one representation gives Index and iterator goodies.

pacak · 2023-06-13T18:50:43Z

src/lib.rs

+    type Output = str;
+
+    fn index(&self, index: usize) -> &Self::Output {
+        &self.content[self.splits[index].clone()]


Index needs to return a reference to so to accommodate both with and without ending - it needs to be decided during construction. Either two different constructors - like I did or using either newtype/type parameter.

pacak · 2023-06-13T18:53:19Z

src/lib.rs

+    pub fn get(&self, index: usize) -> Option<&str> {
+        self.content.get(self.splits.get(index)?.clone())


Yea, for get this works.

pacak added 2 commits March 3, 2022 12:14

implement CachedLines + docs

980ad06

Published 0.1.4

c4844e3

vallentin reviewed Jun 13, 2023

View reviewed changes

pacak commented Jun 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Owning reference to a string with O(1) access to lines #3

Owning reference to a string with O(1) access to lines #3

pacak commented Mar 3, 2022

pacak commented Jun 12, 2023

vallentin commented Jun 12, 2023

vallentin commented Jun 12, 2023

vallentin left a comment

vallentin Jun 12, 2023

pacak Jun 13, 2023

vallentin Jun 12, 2023

pacak Jun 13, 2023

vallentin Jun 13, 2023

pacak Jun 13, 2023

vallentin Jun 13, 2023

pacak Jun 13, 2023

vallentin Jun 13, 2023

vallentin Jun 13, 2023

pacak Jun 13, 2023

vallentin Jun 13, 2023

pacak Jun 13, 2023

vallentin Jun 13, 2023

vallentin Jun 13, 2023

pacak left a comment

pacak Jun 13, 2023

pacak Jun 13, 2023

pacak Jun 13, 2023

pacak Jun 13, 2023

pacak Jun 13, 2023

pacak Jun 13, 2023

	splits: alloc::vec::Vec<Range<usize>>,
	splits: alloc::vec::Vec<LineSpanWithoutText>,

		pub fn get(&self, index: usize) -> Option<&str> {
		self.content.get(self.splits.get(index)?.clone())

Owning reference to a string with O(1) access to lines #3

Are you sure you want to change the base?

Owning reference to a string with O(1) access to lines #3

Conversation

pacak commented Mar 3, 2022

pacak commented Jun 12, 2023

vallentin commented Jun 12, 2023

vallentin commented Jun 12, 2023

vallentin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment