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.
> @@ -57,6 +57,7 @@ typedef enum /*< skip >*/
#define BSE_OBJECT_FLAGS_MAX_SHIFT (16)
/* --- typedefs & structures --- */
+
woot?
> @@ -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<class ObjectImplP> 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 or view it on GitHub:
https://github.com/tim-janik/beast/pull/34#pullrequestreview-149547902_______________________________________________
beast mailing list
[email protected]
https://mail.gnome.org/mailman/listinfo/beast
