The API is used to gracefully terminate lines with a single \n. If the
formatted buffer already ended in \n, it is left alone. If it did not, a
newline character is appended to it. The API, like other sbuf-modifying
routines, is only valid while the sbuf is not FINISHED.
Details
Details
- Reviewers
vangyzen bapt rlibby - Group Reviewers
manpages - Commits
- rS350693: sbuf(9): Add sbuf_nl_terminate() API
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 25459 Build 24080: arc lint + arc unit
Event Timeline
Comment Actions
Logic looks good but I think the terminology is a little misleading. What the flag actually tracks is the last drained byte (mostly, see below). Including "drain" in the name somehow might make it more clear.
Do you plan to add a blurb to sbuf.9? Separately from this patch, it looks like the sbuf.9 blurb about functions that are incompatible with drains needs to be updated: _clear, _len, _bcpy, _cpy, ... any others?
| sys/kern/subr_sbuf.c | ||
|---|---|---|
| 229 | Worth a short comment, since there's actually no '\n' here? | |
| 391–408 | This seems correct, but it's a little non-obvious. It's relying on the idea that if we didn't drain the whole buf, then something remains in the buf, then we don't examine the flag, so it's okay to set it to garbage. I think it would be more clear to set it according to the value of the last byte that we actually drained (s->s_buf[len - 1] after a successful drain call). | |
| 746–749 | Do the predictions really help? | |
Comment Actions
Well, it's a little more complicated than that; it's a yes/no bit for "if the buffer is currently empty, did we drain a non-newline character last?" Unfortunately, the usual pattern for documenting flags only leaves a handful of characters, so I did what I could in the space. If you'd prefer a longer flag comment, I can wrap the line and elaborate.
Do you plan to add a blurb to sbuf.9? Separately from this patch, it looks like the sbuf.9 blurb about functions that are incompatible with drains needs to be updated: _clear, _len, _bcpy, _cpy, ... any others?
I didn't, but that's a good idea, thanks. Will do.
| sys/kern/subr_sbuf.c | ||
|---|---|---|
| 229 | Yes, will add. | |
| 391–408 | The flag is really only backup when the buffer is fully drained and its contents are entirely invalid. If there are any contents in the buffer, the flag doesn't matter. So, in this version it sometimes initializes it unnecessarily (because drain function *might* not drain the entire buffer); in your proposal, it still initializes it unnecessarily (if the drain function did not drain the entire buffer, we don't need to set the flag), just using the contents of a different byte. In both versions the flag value is invalidated by the remaining buffer contents anyway. So my counter-proposal is to just move the existing logic inside the "fast path" if (s->s_len == 0) condition below. Will update. | |
| 746–749 | Probably not. They are inherited from the fmt version. I'm happy to drop 'em. | |
Comment Actions
I might suggest something like SBUF_DRAINATEOL and corresponding _IS macro for consideration. But I don't feel strongly about it. Maybe just enhance the sbuf_nl_terminate comment some and add a comment pointer to it from sbuf_drain.
| sys/kern/subr_sbuf.c | ||
|---|---|---|
| 391–408 | Sounds good. | |
| 746–749 | Your call. IMHO: questionable benefit but definitely ugly. | |
Comment Actions
Looks good to me.
| share/man/man9/sbuf.9 | ||
|---|---|---|
| 127 ↗ | (On Diff #60059) | Just noting, local context uses .Fo / .Fa instead of .Fn here. I think they're equivalent. |