search

TonicAudio / Tonic

Easy and efficient audio synthesis in C++
The Unlicense
523 stars 63 forks source link

Smart pointers, sample tables, ring buffer for input processing #165

Closed ndonald2 closed 11 years ago

ndonald2 commented 11 years ago

This is the same pull request as before, with fixes for all the demos and more illustration of the new features.

Smart Pointers

Dewb commented 11 years ago

I don't yet have enough of a picture of your entire system to speak intelligently about your SynthContext mutex branch. Yeah, let's talk about it offline.

Side question: do you have plans to eventually typedef TonicSmartPointer to std::shared_ptr in C++11 builds?

Dewb commented 11 years ago

(Or subclass shared_ptr. I can see some value in providing a wrapper around make_shared that better suits your library semantics.)

ndonald2 commented 11 years ago

We've been trying to not be fully dependent on C++11 so far, so creating a language-version-dependent class definition might introduce a lot of maintenance cost that we don't necessarily want to incur, but I'm open to learning more about the idea. I'm not familiar enough with make_shared or shared_ptr to know how deep of a change it would be.

morganpackard commented 11 years ago

We want to avoid requiring c++11 for now.

Sent from my iPhone

On May 23, 2013, at 12:39 PM, Michael Dewberry notifications@github.com wrote:

(Or subclass shared_ptr. I can see some value in providing a wrapper around make_shared that better suits your library semantics.)

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/165#issuecomment-18355692 .

morganpackard commented 11 years ago

Sent from my iPhone

On May 23, 2013, at 10:43 AM, "Nick D." notifications@github.com wrote:

This is the same pull request as before, with fixes for all the demos and more illustration of the new features. Smart Pointers

Sweet. I've been thinking about this.

-

Sample Tables and Input

Misc

You can merge this Pull Request by running

git pull https://github.com/TonicAudio/Tonic ND-SampleTables+Input

Or view, comment on, or merge it at:

https://github.com/TonicAudio/Tonic/pull/165 Commit Summary

File Changes

Patch Links:

morganpackard commented 11 years ago

And I'm not familiar enough to know what the benefits would be.

Sent from my iPhone

On May 23, 2013, at 1:03 PM, "Nick D." notifications@github.com wrote:

We've been trying to not be fully dependent on C++11 so far, so creating a language-version-dependent class definition might introduce a lot of maintenance cost that we don't necessarily want to incur, but I'm open to learning more about the idea. I'm not familiar enough with make_shared or shared_ptr to know how deep of a change it would be.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/165#issuecomment-18357154 .

Dewb commented 11 years ago

Yeah, having to maintain separate C++03 and 11 versions of a core part of your infrastructure is a bad idea, that was a silly thing to suggest.

A more realistic suggestion would be to switch to boost::shared_ptr or tr1::shared_ptr across the board. Both are available without C++11, and they're all more or less identical.

I see two benefits:

  1. You get to use the reference-counted but non-lifespan-preserving companion class weak_ptr, which allows you to break cycles and do other fancy things. Not much need for it now but if you want to let people create feedback paths, it may come up.
  2. You get the benefit of using someone else's tested and debugged smart pointer library. Not that there's a problem currently -- from what I understand, reference counted smart pointers don't have that many buried gotchas, and you guys appear to have a solid implementation. But it's a virtual certainty that you'll discover other things it needs to do as time goes on. On the other hand it might be sensible to wait until that starts to happen first; the existing implementation may be enough for the foreseeable future.
morganpackard commented 11 years ago

Thanks for your thoughts, Mike. Currently, Tonic has no dependencies at all, which I think is pretty awesome. Requiring someone to install boost would be a pretty big downside IMO. Though I guess we could just include the boost files in our project, right? -m-

On Thu, May 23, 2013 at 4:37 PM, Michael Dewberry notifications@github.comwrote:

Yeah, having to maintain separate C++03 and 11 versions of a core part of your infrastructure is a bad idea, that was a silly thing to suggest.

A more realistic suggestion would be to switch to boost::shared_ptr or tr1::shared_ptr across the board. Both are available without C++11, and they're all more or less identical.

I see two benefits:

  1. You get to use the reference-counted but non-lifespan-preserving companion class weak_ptr, which allows you to break cycles and do other fancy things. Not much need for it now but if you want to let people create feedback paths, it may come up.
  2. You get the benefit of using someone else's tested and debugged smart pointer library. Not that there's a problem currently -- from what I understand, reference counted smart pointers don't have that many buried gotchas, and you guys appear to have a solid implementation. But it's a virtual certainty that you'll discover other things it needs to do as time goes on. On the other hand it might be sensible to wait until that starts to happen first; the existing implementation may be enough for the foreseeable future.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/165#issuecomment-18370026 .

Morgan Packard cell: (720) 891-0122 aim: mpackardatwork twitter: @morganpackard

Dewb commented 11 years ago

Yeah, boost::shared_ptr is header only and can be included. It might make more sense to use std::shared_ptr in C++11 and tr1::shared_ptr in other environments. The advantage of using boost::shared_ptr is that it would be the same code everywhere; the advantage of using std/tr1 is that you don't have to include anything.

morganpackard commented 11 years ago

Any potential for conflicts if someone uses Tonic in another project which also uses boost? (I'm green enough with C++ that i'm not really sure what library conflicts look like in C++).

My general rule is: keep stuff super simple unless there's a good reason not to. Using boost feels like a step toward more complexity at this point, though I can see that if we started to use smart pointers in lots of different ways, boost might become the simpler way to go.

-m-

On Thu, May 23, 2013 at 5:19 PM, Michael Dewberry notifications@github.comwrote:

Yeah, boost::shared_ptr is header only and can be included. It might make more sense to use std::shared_ptr in C++11 and tr1::shared_ptr in other environments. The advantage of using boost::shared_ptr is that it would be the same code everywhere; the advantage of using std/tr1 is that you don't have to include anything.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/165#issuecomment-18372410 .

Morgan Packard cell: (720) 891-0122 aim: mpackardatwork twitter: @morganpackard

ndonald2 commented 11 years ago

In the midst of all this I actually found and fixed a bug I introduced in this branch where the pcount of a new smart pointer instance was not being correctly initialized...

I still think we're doing OK with our own implementation, but obviously it takes some serious profiling and testing to ensure that everything is working correctly under the hood.

That being said, I'm not against using tr1::shared_porinter or std::shared_ptr. It wouldn't be too tough to do a compile-time typedef based on which one is available for a given platform. We wouldn't be introducing anything that isn't part of standard c++03 implementations, and we'd have the extra reliability of an optimized, stable smart pointer. However, I'd like to learn more about shared_ptr in general to see if what we're doing by adding templates on top, etc, would still be feasible. I don't particularly like the idea of using the structure dereference operator that shared_ptr seems to implement by default to access the underlying object (->) but there may be a way to obfuscate that in a subclass.

Dewb commented 11 years ago

Here's what Cinder used to do back before Xcode had C++11 support:

#if defined( _MSC_VER ) && ( _MSC_VER >= 1600 ) || defined( _LIBCPP_VERSION )
    #include <memory>
#elif defined( CINDER_COCOA )
    #include <tr1/memory>
    namespace std {
        using std::tr1::shared_ptr;
        using std::tr1::weak_ptr;       
        using std::tr1::static_pointer_cast;
        using std::tr1::dynamic_pointer_cast;
        using std::tr1::const_pointer_cast;
        using std::tr1::enable_shared_from_this;
    }
#else
...

That allowed them to use std::shared_ptr in their code, regardless of whether the compiler's library had one in the std or the tr1 namespace. (The tr1 namespace is where pre-C++11 compilers kept the experimental C++0x features for the few years the standard was being finalized.) And in the ellipsis, they would bring in the Boost version if the environment didn't have tr1.

You wouldn't need to worry about Boost if you assume that the compiler supports either TR1 or C++11. I believe the last several years' versions of MSVC, GCC, LLVM and Clang all meet that definition.

ndonald2 commented 11 years ago

Closing, re-opening against development branch