On Fri, Apr 11, 2014 at 12:53:08PM +0200, Pavel Březina wrote:
On 04/10/2014 11:03 AM, Jakub Hrozek wrote:
>The attached patch implements introspecting the sbus interfaces as
>tracked by #2234.
>
>There is one part of the patch I dislike, but I wanted to get other
>opinions, too -- the discard_const in sbus_message_handler(). I was
>going back and forth on whether sbus_introspect() should be an sbus
>handler like the other handlers or whether I should special case it (and
>then pass the interface directly and not through a void pointer).
>Special casing the introspection might be a bit cleaner, but would cause
>some duplication in sbus_message_handler().
In my opinion it is correct to make it a standard sbus handler. I
think that it is ok to use discard_const() in this situation,
however it can be avoided. Cleaner solution would be to create a new
context for private data, e.g.:
struct sbus_introspect_ctx {
const struct sbus_interface_meta *iface;
};
>The other question I have is whether there should be a way to mark an
>interface as not introspectable? I can't think of a reason, so by
>default all interfaces can be introspected.
We should deal with this question when we actually hit an interface
that must not be introspectable for some reason.
>The introspection also doesn't generate DocString annotations yet, I
>will send a patch for that separately.
The code looks good and it works splendidly! Good job!
I'm leaving the discard_const issue on your decision, I don't think
it is really necessary to avoid it in this case.
It's an ack from me.
Hi,
I think the sbus_introspect_ctx context is a good idea. It requires some
special casing in sbus_message_handler(), though..
Attached are two patches, one is the modified patch including the
sbus_introspect_ctx, the other is just the interdiff for easier review.
Thanks for the review!