sys: ringbuffer: `rb_add_elements()` don't overwrite existing content by Kijewski · Pull Request #933 · RIOT-OS/RIOT

Kijewski · 2014-03-28T21:19:52Z

rb_add_elements() is undocumented and currently unused. I'd like to use it. My problem is that the function overwrites existing content. Hence it doesn't return how many elements were added.
The corresponding get function rb_get_elements() only reads as many elements as there are stored, and returns the number of actually read elements.

Are there objections to change the behavior of rb_add_elements() not to overwrite existing elements?

Kijewski · 2014-03-28T21:21:27Z

Added commits.

haukepetersen · 2014-03-31T12:08:50Z

sys/include/ringbuffer.h

you change this to the worse, @file FILENAME.[c|h] is the preferred style here...

The name is filled in automatically. Declaring the name makes finding the source of the definition more difficult in the generated HTML.

haukepetersen · 2014-03-31T12:14:47Z

+1 for the changes!

Just seems to me the implementation is not yet complete?!

Kijewski · 2014-03-31T12:29:00Z

It is complete. I only changed rb_add_elements() and documented the existing functions.
If the implementation does not fit the documentation, then the comments need to be fixed.

miri64 · 2014-04-14T12:56:19Z

sys/include/ringbuffer.h

Align with other comments.

kaspar030 · 2014-04-14T12:59:59Z

Also +1 for the changes!

miri64 · 2014-04-14T13:00:03Z

sys/include/ringbuffer.h

Maybe, while you're at it: rename ringbuffer_add_element

kaspar030 · 2014-04-14T13:03:43Z

@authmillenon really, ringbuffer_*? Why write out everything? I prefer the short names.

miri64 · 2014-04-14T13:09:49Z

Because rb_* could refer to anything and thus can lead to problems in readability (and portability).

Kijewski · 2014-04-14T13:13:57Z

I like longer names, too. Even my smallish editor Geany supports autocompletion, and every major editor (vim, emacs, Eclipse, …) does, too.

kaspar030 · 2014-04-14T13:17:12Z

Do we like longer names because our editor supports autocompletion?

I like shorter names because they IMHO increase readability. Guess you would state the opposite?

kaspar030 · 2014-04-14T13:21:17Z

I'm even for renaming rb_add_elements to rb_add and get rid of the single byte version altogether for external use (or change it to rb_addone).

miri64 · 2014-04-14T13:31:20Z

It's more a "where does this come from" readability, than a "This code line is shorter then 50 chars" readability I meant.

miri64 · 2014-04-14T13:50:09Z

If we call the module rb I would not have any problems with rb_ ;-) But I do not think this would be usefull.

kaspar030 · 2014-04-14T13:53:18Z

why not call the module ringbuffer so it's clear what it contains. Call the type ringbuffer_t because it is only used in definitions and signature. Call the functions rb_* so we don't have to type our fingers wound. As soon as we have anything prone to collide with rb_*, we can rediscuss our convention here.

ringbuffer_init could be renamed, though...

kaspar030 · 2014-04-14T13:54:26Z

Let's prefer practical inconsistency over academic consistency.

Kijewski · 2014-04-14T14:00:14Z

Let's prefer practical inconsistency over academic consistency.

miri64 · 2014-04-14T14:03:03Z

sys/include/ringbuffer.h

OlegHahm · 2014-04-14T15:43:51Z

#define rb_ ringbuffer_

? ;-)

miri64 · 2014-04-15T10:40:22Z

Can you rebase please? :D I'd like to use this PR for my packet buffer but am not able to write unittests.

Kijewski · 2014-04-15T11:56:53Z

@authmillenon, rebased.

Kijewski · 2014-05-03T18:27:57Z

This PR was superseded by #1101.

OlegHahm · 2014-05-04T11:35:35Z

Close?

Kijewski · 2014-05-04T14:06:44Z

I wanted to wait if someone has some final comment, or if there is a strong opposition against the other PR. Right now there is not much comment on #1101. : )

Kijewski · 2014-05-08T13:34:00Z

Replaced by #1101. I'm closing this PR as I won't rebase this branch.

Kijewski added the question label Mar 26, 2014

Kijewski assigned kaspar030 and haukepetersen and unassigned kaspar030 Mar 26, 2014

Kijewski added enhancement and removed question labels Mar 28, 2014

This was referenced Mar 28, 2014

sys: Add generic pipe #935

Merged

sys: Uncouple UART from POSIX #945

Closed

haukepetersen reviewed Mar 31, 2014
View reviewed changes

miri64 reviewed Apr 14, 2014
View reviewed changes

sys/include/ringbuffer.h Outdated

Copy link
Copy Markdown

Member

miri64 Apr 14, 2014

Choose a reason for hiding this comment

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

Align with other comments.

miri64 reviewed Apr 14, 2014
View reviewed changes

sys/include/ringbuffer.h Outdated

Copy link
Copy Markdown

Member

miri64 Apr 14, 2014

Choose a reason for hiding this comment

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

Maybe, while you're at it: rename ringbuffer_add_element

miri64 reviewed Apr 14, 2014
View reviewed changes

sys/include/ringbuffer.h Outdated

Copy link
Copy Markdown

Member

miri64 Apr 14, 2014

Choose a reason for hiding this comment

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

*from

Kijewski mentioned this pull request Apr 15, 2014

sys: rename ringbuffer functions #1011

Closed

Kijewski changed the title ~~sys: rb_add_elements() don't overwrite existing content~~ sys: ringbuffer: rb_add_elements() don't overwrite existing content Apr 15, 2014

Kijewski added 3 commits April 29, 2014 16:48

Proper rb_add_elements() implementation

099d895

Add documentation for ringbuffer

20c51d0

Remove old comments from ringbuffer.c

e3c82c5

Kijewski mentioned this pull request May 3, 2014

ringbuffer: versatility and optimizations #1101

Merged

Kijewski closed this May 8, 2014

Kijewski deleted the issue-933 branch May 8, 2014 16:17

Kijewski modified the milestone: Release 2014.05 May 14, 2014

Conversation

Kijewski commented Mar 28, 2014

Uh oh!

Kijewski commented Mar 28, 2014

Uh oh!

haukepetersen Mar 31, 2014

Choose a reason for hiding this comment

Uh oh!

Kijewski Mar 31, 2014

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Mar 31, 2014

Uh oh!

Kijewski commented Mar 31, 2014

Uh oh!

miri64 Apr 14, 2014

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Apr 14, 2014

Uh oh!

miri64 Apr 14, 2014

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Apr 14, 2014

Uh oh!

miri64 commented Apr 14, 2014

Uh oh!

Kijewski commented Apr 14, 2014

Uh oh!

kaspar030 commented Apr 14, 2014

Uh oh!

kaspar030 commented Apr 14, 2014

Uh oh!

miri64 commented Apr 14, 2014

Uh oh!

miri64 commented Apr 14, 2014

Uh oh!

kaspar030 commented Apr 14, 2014

Uh oh!

kaspar030 commented Apr 14, 2014

Uh oh!

Kijewski commented Apr 14, 2014

Uh oh!

miri64 Apr 14, 2014

Choose a reason for hiding this comment

Uh oh!

OlegHahm commented Apr 14, 2014

Uh oh!

miri64 commented Apr 15, 2014

Uh oh!

Kijewski commented Apr 15, 2014

Uh oh!

Kijewski commented May 3, 2014

Uh oh!

OlegHahm commented May 4, 2014

Uh oh!

Kijewski commented May 4, 2014

Uh oh!

Kijewski commented May 8, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants