Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BSE: bse_cast - an API suggestion of how to handle bse object casts #34

Closed
wants to merge 1 commit into from

Conversation

swesterfeld
Copy link
Collaborator

During fixes for #32 #24 you introduced manual null pointer checks for as<TrackIfaceP> and so on. I don't believe that this is the best approach; so I here is a suggestion of how I think it should be done.

I would recommend replacing each invocation of as<> with bse_cast<>, even if in some cases this will be a little longer:

casting track object
old: track ? track->as<TrackIfaceP>() : NULL;
new: bse_cast<TrackIfaceP> (track);

casting this object
old: as<TrackIfaceP>();
new: bse_cast<TrackIfaceP> (this);

However, I still think my version is the better API, because

  • you cannot forget the null pointer check by accident
  • it is a little more intuitive to see that we're just casting here

Its a bit of work to do the details and replace all cases where this is used, but if you want to go this way, I can provide a complete patch which eliminates as<>() completely.

Copy link
Owner

@tim-janik tim-janik left a comment

Choose a reason for hiding this comment

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

I've added some review comments inline (trying this out in github to see if that's a helpful feature). The most important point is the fact that as<> (or a new bse_cast<>) will be temporary measures that we ultimately want to get rid of in libbse.

@@ -57,6 +57,7 @@ typedef enum /*< skip >*/
#define BSE_OBJECT_FLAGS_MAX_SHIFT (16)

/* --- typedefs & structures --- */

Copy link
Owner

Choose a reason for hiding this comment

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

woot?

@@ -88,6 +89,15 @@ struct BseObject : GObject {
}
};

template<class ObjectImplP>
ObjectImplP bse_cast (BseObject *object)
Copy link
Owner

Choose a reason for hiding this comment

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

  1. return values should always go on their own line, so here, you'd write:

    template ObjectImplP
    bse_cast (BseObject *object)

  2. There're a lot more back and forth conversions going on between C and C++ objects. E.g. we have as<> variants for BseItem* -> Bse::ItemImpl, -> Bse::ItemIface, -> Bse::ItemImplP, Bse::ItemIfaceP, and also for going from each of those to back to BseItem*. A bse_cast<> macro that wants to handle those conversions should use SFINAE measures (or static_assert - easier but not always possible) to a) only convert BseObject* or derived into C++ types, b) convert only Bse::ObjectIface, Bse::ObjectImpl or shared_ptr variants thereof into the corresponding BseObject* C type.

I honestly don't know how much work that involves to get right. You might want to give it a shot, but please keep in mind that after we've completed the SfiProxy property -> C++ accessor and signal -> Aisa::Event migrations, there's a huge BSE-internal cleanup phase pending where we'll attempt to merge the existing C structs with the Aida C++ classes. So ultimately, the as<> methods or any bse_cast<> variant thereof will be a temporary measure.

@tim-janik
Copy link
Owner

Ultimately, the Bse* C-Objects are scheduled for removal now, so I guess this isn't needed anymore.

@tim-janik tim-janik closed this Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants