search

Cysharp / R3

The new future of dotnet/reactive and UniRx.
MIT License
2.1k stars 92 forks source link

Change RingBuffer class visibility to internal. #211

Closed filzrev closed 3 months ago

filzrev commented 4 months ago

Currently RingBuffer class is located under R3.Internal namespace. But it's exposed as public visibility.

Is there any specific reason to use public visibility? If not, please consider change visibility to internal class.

ninjaoxygen commented 4 months ago

Ringbuffer is useful to have exposed for anyone that wants to implement custom subjects similar to the existing R3 ones, you end up using the R3.Internal classes.

I think it makes sense to have it public in R3.Internal, then it is conveniently separated away from the public API but available for custom implementations.

filzrev commented 4 months ago

I knows the usefulness of using existing RingBuffer implementation. But I thought the name is too generic for this purpose.

For example. When using R3 with CySharp/ObservableCollections. Two RingBuffer candidates are suggested by intellisense for namespace import.

And it's confusing that R3's RingBuffer implementation automatically increase internal buffer size. instead of fixe-size circular buffer.

Personally, either of the following approach is preffered.

  1. Change RingBuffer visibility to internal.
  2. Change RingBuffer class name to self-descriptive redundant name. And move to R3 namespace.

Current RingBuffer implementation assume maximum internal buffer size as 1073741824. So following issue exists if directory using RingBuffer implementations.

  1. When specifying initial capacity size larger than 1073741824. Initial internal buffer size is set smaller value. -> It should use 1073741824 as max buffer size. or throw ArgumentRangeException if larger value specified.

  2. When add items over 1073741824. it throw OverflowException -> It need to manually implement trim operation like ReplaySubject.cs without Trimming(Subscribe) operations.

neuecc commented 3 months ago

Thank you for your opinion. For R3, I think it's preferable to make this internal, so I will change it to internal.

filzrev commented 3 months ago

Thanks for changing the visibility for RingBuffer.

I though existing user who using RingBuffer implementation is recommended to use ObservableCollections of Cysharp/ObservableCollections's RingBuffer implementation instead,