Skip to content

Added an offset to search(io::IOBuffer, delim, offset), so that#4485

Closed
kmsquire wants to merge 1 commit intomasterfrom
kms/iobuffer_search_offset
Closed

Added an offset to search(io::IOBuffer, delim, offset), so that#4485
kmsquire wants to merge 1 commit intomasterfrom
kms/iobuffer_search_offset

Conversation

@kmsquire
Copy link
Copy Markdown
Member

@kmsquire kmsquire commented Oct 12, 2013

the search can pick up after the buffer is filled further.

One use case was changed to use this. Other uses exist outside of Base.

@kmsquire
Added an offset to search(io::IOBuffer, delim, offset), so that
a626440 
the search can pick up after the buffer is filled further.
@JeffBezanson
Copy link
Copy Markdown
Member

JeffBezanson commented Oct 12, 2013

Seems fine. I wonder whether offset should be 1-indexed. Passing a byte count makes sense, but it might be too much of a departure from our normal conventions.

@kmsquire
Copy link
Copy Markdown
Member Author

kmsquire commented Oct 12, 2013

I thought about that. I went this route since IOBuffer implements an IO interface and already uses 0-based indexing--seek(io, 0) goes the beginning of the buffer, not seek(io, 1), although we do have seekstart().

But anyway, I'll change it and see what it looks like.

@JeffBezanson
Copy link
Copy Markdown
Member

JeffBezanson commented Oct 12, 2013

Yeah, that's a good point. I'm not sure about this one.

@kmsquire
Copy link
Copy Markdown
Member Author

kmsquire commented Oct 12, 2013

All of the uses of search(io::IOBuffer, x) (in client.jl, stream.jl, and iobuffer.jl itself) are using it to get a count of the number of bytes to read, starting at the current pointer in the buffer and including the delimiter. So

So while it's still searching for something, the question really being asked is "how many bytes are there up to and including the next x in this stream". Since this is a question being asked about a stream, I think an offset makes more sense than an index, so I'm inclined to leave it as 0-based.

Also, since there's a general desire to have semantically different ideas have different function names, perhaps this function should be renamed? One possibility would be nb_available(io::IOBuffer, upto) or nb_available_to(io::IOBuffer, upto)

(Which is a rather unfortunate name... perhaps that could be renamed to bytesavailable() or bytesavail(), and the corresponding name here changed?)

@kmsquire
Copy link
Copy Markdown
Member Author

kmsquire commented Oct 12, 2013

(Or for readability, bytes_avail() or bytes_available())

@JeffBezanson
Copy link
Copy Markdown
Member

JeffBezanson commented Oct 12, 2013

+1 to such a renaming

@kmsquire
Copy link
Copy Markdown
Member Author

kmsquire commented Oct 12, 2013

Any preference?

@kmsquire
Copy link
Copy Markdown
Member Author

kmsquire commented Oct 12, 2013

Or rather, which renaming?

@StefanKarpinski
Copy link
Copy Markdown
Member

StefanKarpinski commented Nov 19, 2013

bump

@kmsquire
Copy link
Copy Markdown
Member Author

kmsquire commented Nov 19, 2013

Any thoughts on a good name? I'm not enamoured with the current name or my previous suggestions. The name should probably reflect "number of bytes available until the next delimiter".

@kmsquire
Copy link
Copy Markdown
Member Author

kmsquire commented Nov 19, 2013

If no other suggestions come up, I'd say this is a minimal change and can probably just be applied. It really only affects one use of search, which is updated here.

JeffBezanson
JeffBezanson reviewed Dec 19, 2013
View reviewed changes
base/stream.jl
Copy link
Copy Markdown
Member

@JeffBezanson JeffBezanson Dec 19, 2013

Choose a reason for hiding this comment

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

Is this really correct? I think there is a chance the libuv layer will end up compacting the buffer before we go around the loop again. @loladiro

Copy link
Copy Markdown
Member Author

@kmsquire kmsquire Dec 19, 2013

Choose a reason for hiding this comment

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

Hmm... that could be. Also cc: @vtjnash

Copy link
Copy Markdown
Member

@vtjnash vtjnash Dec 19, 2013

Choose a reason for hiding this comment

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

The interface functions to the IOBuffer object are not effected by compacting. The failure condition here is the assumption that there are no other consumers of the stream. mark may be helpful here to validate that assumption?

Copy link
Copy Markdown
Member Author

@kmsquire kmsquire Jan 23, 2014

Choose a reason for hiding this comment

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

mark would maintain all data in the buffer after the mark, and this PR could be rewritten using mark.

Unfortunately, #3656 is stuck right now because of my lack of understanding as to why it's affecting the build, so mark isn't yet available.

@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Dec 19, 2013

+2 to renaming nb_available

@StefanKarpinski
Copy link
Copy Markdown
Member

StefanKarpinski commented Jan 23, 2014

bump. it's unclear to me what the status of this is. the nb_available rename discussion seems incidental.

@JeffBezanson
Copy link
Copy Markdown
Member

JeffBezanson commented Jun 2, 2014

Closing due to doubts about correctness with multiple readers.

@kmsquire kmsquire deleted the kms/iobuffer_search_offset branch August 10, 2014 20:04
Keno pushed a commit that referenced this pull request Nov 25, 2025
@DilumAluthgeBot @IanButterworth
🤖 Bump Pkg stdlib 2141bf17f → 1e90f07f9 (#60237)
a4d2b6a 
Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: master
Julia branch: master
Old commit: 2141bf17f
New commit: 1e90f07f9
Julia version: 1.14.0-DEV
Pkg version: 1.14.0
Bump invoked by: @IanButterworth
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@2141bf1...1e90f07

```
$ git log --oneline 2141bf17f..1e90f07f9
1e90f07f9 Fix is_stdlib uses when julia_version is used (#4534)
3f0c20ec7 Ensure `@stdlib` is on `LOAD_PATH` for `.pkg/` hooks (#3167)
eaaecc75b Bump actions/checkout from 5 to 6 in the all-actions group (#4530)
56e16ea22 Fix more test cases for base CI (#4531)
0369c9363 Isolate more tests (#4528)
3bc7bff5c Propagate syntax version from Project to Manifest (#4520)
77db067f8 Publicize `Apps` (#4524)
f370106a4 CI: Fix duplicate CI jobs on backports PRs (#4521)
1918a7624 ensure REPLExt is precompiled before we disallow it (#4517)
901e5f0f2 finish deprecating collect_delay (#4516)
854ac75ed Use `Sys.STDLIB` instead of hardcoding path to Julia stdlib (#4515)
0c7013bfa Fix some tests when run in base (#4499)
31ae9601a  fix: work around string indexing issues in windows drive letter detection
bf67f6342 test: isolate more tests that call subprocesses (#4504)
7302e1960 avoid some overhead from Tar.jl when unpacking the registry (#4505)
101997edf keep using gzip on Windows for the registry (#4496)
db3e6f0b4 use the same value of `--check-bounds` as the process (#4494)
e0af33508 only propagate paths in sources for fixed packages if they actually exist (#4503)
d3c44751b Fix formatting of warning admonition (#4498)
10fc1126a show the original libgit2 error message which includes the `insteadOf` url (#4501)
7874cbd4f Merge pull request #4500 from JuliaLang/kc/substring
88a46e68f fix building packages on app add (#4491)
87a86dcfc use shallow clones for registry/packages (when supported by LibGit2) (#4487)
efe1eaf7a stop "uncompressing" registry data (#4470)
ca5b0896a Improve docs for test-specific deps with workspace approach (#4490)
c1fee0e60 fix relative paths in Apps for dev (#4481)
e6b1d0b24 Bump version from 1.13.0 to 1.14.0 (#4485)
```

Co-authored-by: IanButterworth <1694067+IanButterworth@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kmsquire @JeffBezanson @StefanKarpinski @vtjnash