On 04/14/2014 04:17 PM, Jakub Hrozek wrote:
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!
Ack.