Various fixes

[matthijskooijman]

Heres a few improvements to the code that I needed, as well as a fix to the library.properties file. I didn't bump the version number, I'll let that up to you.

[wizard97]

Everything looks good, thanks for the merge, a few questions though.

1. Why do you think there needs to be seperate alias methods for the same thing? (push(), append(), add())

2. I was hoping you would fork the dev branch and apply the changes to that branch. The dev branch has a better way to handle atomic operations. I was planning on merging dev into master shortly and releasing a newer version. Would you mind adding these changes into the dev branch instead of master?

[matthijskooijman]

Why do you think there needs to be seperate alias methods for the same thing? (push(), append(), add())

When the buffer allows modifying either end, add() is ambiguous as it doesn't make clear what it is doing exactly. Hence, append() and prepend() are better, since they are more explicit. Then, in cases where the buffer is used as a stack, push() and pop() make sense to be used together. So the point is mostly to allow writing more readable code when using this library.

I was hoping you would fork the dev branch and apply the changes to that branch. The dev branch has a better way to handle atomic operations. I was planning on merging dev into master shortly and releasing a newer version. Would you mind adding these changes into the dev branch instead of master?

I hadn't seen the dev branch until after I created this PR, but I've now rebased it on top of dev and updated this PR. I can't edit this PR to merge into dev, so it now also includes your commits (you'll probably need to manually merge this PR then). I also added one more commit that lets pull(Type&) call pull(Type*) instead of duplicating the implementation and one commit that documents pull(Type&), and I updated the rest of my commits to match your changes.

[wizard97]

When the buffer allows modifying either end, add() is ambiguous as it doesn't make clear what it is doing exactly. Hence, append() and prepend() are better, since they are more explicit. Then, in cases where the buffer is used as a stack, push() and pop() make sense to be used together. So the point is mostly to allow writing more readable code when using this library.

I understand now what your intensions are, my only concern is now just looking at all the methods, the API is kinda confusing. I think the README needs to be revised to make it clear which methods are relevant for FIFO (queue) usage and which are relevant for LIFO (stack) usage. Then all the more complicated methods can go under an advanced section (peek(), prepend(), etc...). Another issue is at some point these changes will need to be merged into the C version of the library, and each method requires a pointer (basically to act as a vtable), so each alias method basically wastes 4 bytes in the C version.

I'm almost tempted at this point to make two new classes that inherit from some RingBuf base class: A FIFO class and a LIFO class. The FIFO class will have the queue related aliases and the LIFO class will have the stack related aliases. Let me know what you are thinking, as this is the direction I'm thinking is best right now.

[matthijskooijman]

and each method requires a pointer (basically to act as a vtable), so each alias method basically wastes 4 bytes in the C version.

Right. For the aliases, you could probably solve this using an anonymous union (to have multiple names for the same pointer), but you'd still have 4 methods of which you'll likely not using all. However, looking at the C implementation, why are you using function pointers for a vtable anyway? It looks like these pointers are always pointing to the same methods anyway?

I'm almost tempted at this point to make two new classes that inherit from some RingBuf base class: A FIFO class and a LIFO class. The FIFO class will have the queue related aliases and the LIFO class will have the stack related aliases. Let me know what you are thinking, as this is the direction I'm thinking is best right now.

If you only add the methods relevant to either FIFO or LIFO (so only one addition and one removal method), then you lose flexibility for a mixed usecase (which is why I started writing this in the first place, since I was using a LIFO buffer, but sometimes needed FIFO behaviour). I would think that splitting into two classes would be more confusing than having a few extra methods (even with the duplicate names). A bit of documentation explaining what the idea is would also help, I think.

I'm not really attached to the aliases, though, so if you think it is better to just keep one method per operation, that's also fine by me. That would give prepend, add, pull, pop. Ideally, I would make their names more explicit, something like: append, prepend, remove_first and remove_last. (IMHO pop doesn't really make sense without push, and pull isn't all that clear to me either). For compatibility you would still need to have add and pull aliases, though, so might as well have the full set (or use the four explicit methods as the primary ones and add compatibility aliases and mark them as deprecated).

[wizard97]

Right. For the aliases, you could probably solve this using an anonymous union (to have multiple names for the same pointer), but you'd still have 4 methods of which you'll likely not using all. However,

I have actually never heard of anonymous unions until now, I'm much more inclined now to have aliases since it possible to remove the overhead of them.

looking at the C implementation, why are you using function pointers for a vtable anyway? It looks like these pointers are always pointing to the same methods anyway?

Initially part of the intention of this library was to show how you can apply OOP principles using only C. At this point there is no reason for having these virtual methods, and they still exist solely for ease in programming and backwards compatibility.

If you only add the methods relevant to either FIFO or LIFO (so only one addition and one removal method), then you lose flexibility for a mixed usecase (which is why I started writing this in the first place, since I was using a LIFO buffer, but sometimes needed FIFO behaviour).

I understand you loose flexibility by not incorporating all these methods, but personally I have not convinced myself of a use case where you would need to have access to LIFO and FIFO methods on the same buffer. It seems to me you are either using a Queue (FIFO) or a Stack (LIFO). Can you give me an example of your use case that shows why it is critical to have access to both of these sets of methods at once? Part of design goal of this library is to make it as simple to use as possible, and methods that will only be used in 1% of use cases at the expense of complexity are not worth IMHO.

I'm not really attached to the aliases, though, so if you think it is better to just keep one method per operation, that's also fine by me. That would give prepend, add, pull, pop. Ideally, I would make their names more explicit, something like: append, prepend, remove_first and remove_last. (IMHO pop doesn't really make sense without push, and pull isn't all that clear to me either). For compatibility you would still need to have add and pull aliases, though, so might as well have the full set (or use the four explicit methods as the primary ones and add compatibility aliases and mark them as deprecated).

I actually completely agree with you on this one. pop() and push() need to both exist if they do at all. And add() and remove() for the queue interface. I do like the aliases given that we can use anonymous unions to remove the overhead. Then the old names can be deprecated and then do a major version bump.

I would think that splitting into two classes would be more confusing than having a few extra methods (even with the duplicate names). A bit of documentation explaining what the idea is would also help, I think.

The idea would be you would have some base RingBuf class which implements all the basic prepend(), append(), pull(), etc... Then you have a Queue class that inherits and implements add() and remove() methods. And a Stack class that inherits for the base class and implements push() and pop().

At this point the question is should people have access to both stack and queue methods simultaneously. I personally think they should be separated, but if you can convince me of use cases when you need both then we can do that. Once this is figured out I will go ahead and merge the changes and draft a new release.

[matthijskooijman]

Can you give me an example of your use case that shows why it is critical to have access to both of these sets of methods at once?

The usecase I had was the following. I was building a list of measurements, to be sent using a radio protocol. For redundancy, I would periodically add one measurement to the list (push()), removing the oldest one to make room if needed (pull()). However, sometimes the sending would fail, and I would retry after a while. However, when retrying, I want to replace the most recent measurement with a fresh one (so, pop() and then push()). Hence, I needed three of the four operations.

Note that by now, I've redesigned the operation of my sketch, so I'm not currently using this anymore. If the above example does not convince you, feel free to adopt the split-class approach (but I probably won't have time to implement that).

[wizard97]

Ok, I think you convinced me enough. I have just been thinking about the cleanest solution that gives you these advanced features without making the common case too complex. I'm not a huge fan of the derived split class approach either...

I think what I'm going to do is as follows:
I'm going to incorporate the changes you proposed into the RingBuf class, but then I'm going to create a RingQueue and RingStack class (or something similar) that instantiates a private instance of the RingBuf class. Then I will add the corresponding Queue/Stack methods to each class that calls the correct corresponding method on the internal RingBuf. I will inline all these methods because they are essentially just aliases to remove the overhead.

This way, if you need the advanced features you can use a raw RingBuf class, otherwise (and in most cases) you use the correct RingStack or QueueStack class.

[matthijskooijman]

One alternative springs to mind: Make RingQueue and RingStack subclasses of RingBuf, and make the methods you do not want private? I think you can "remove" methods that way.

[matthijskooijman]

Or, perhaps privately inherit from them and use the C++11 using keyword to explicitely inherit the methods and constructors you need (not sure if this is actually possible).

I'm not sure if any of these are better per se, it's just some options that came to mind.

[wizard97]

Actually I think exactly what we are looking for is private C++ inheritance. The RingStack and RingQueue will both inherit from RingBuf privately. This will make all the public RingBuf methods private and then I will add the appropriate public methods to both RingStack and RingQueue that call the appropriate (now private) method.

I will get this merged in when I get a chance!