[tim-janik/beast] BSE: bse_cast - an API suggestion of how to handle bse object casts (#34)

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[tim-janik/beast] BSE: bse_cast - an API suggestion of how to handle bse object casts (#34)

Stefan Westerfeld-2

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.


You can view, comment on, or merge this pull request online at:

  https://github.com/tim-janik/beast/pull/34

Commit Summary

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

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"BSE: bse_cast - an API suggestion of how to handle bse object casts (#34)"}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/34"}}}</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: [tim-janik/beast] BSE: bse_cast - an API suggestion of how to handle bse object casts (#34)

Gnome - Beast mailing list

@tim-janik requested changes on this pull request.

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.


In bse/bseobject.hh:

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

woot?


In bse/bseobject.hh:

> @@ -88,6 +89,15 @@ struct BseObject : GObject {
   }
 };
 
+template<class ObjectImplP>
+ObjectImplP bse_cast (BseObject *object)
  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.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"PERSON","message":"@tim-janik requested changes on #34"}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/34#pullrequestreview-149547902"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/34#pullrequestreview-149547902", "url": "https://github.com/tim-janik/beast/pull/34#pullrequestreview-149547902", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "@tim-janik requested changes on 34", "sections": [ { "text": "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\u003c\u003e (or a new bse_cast\u003c\u003e) will be temporary measures that we ultimately want to get rid of in libbse.", "activityTitle": "**Tim Janik**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@tim-janik", "facts": [ ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/34#pullrequestreview-149547902" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 311090265\n}" } ], "themeColor": "26292E" } ]</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast