[lttng-dev] [REVIEW REQUEST babeltrace] Provide generic event payload accessors to the Python bindings

[lttng-dev] [REVIEW REQUEST babeltrace] Provide generic event payload accessors to the Python bindings

[Jérémie Galarneau]

Hi all,

I'd like to submit a review request for commits 34cf520 .. 3c5bb23 of
my bindings/python branch[1]. These are patches that were originally
contributed by Xiaona Han, that I have reviewed and to which I have
made minor changes (fixed typos and clarified some commit messages).

Thanks,
J?r?mie

[1] git://github.com/jgalar/babeltrace.git -b bindings/python
Web: https://github.com/jgalar/babeltrace/tree/bindings/python

-- 
J?r?mie Galarneau
EfficiOS Inc.
http://www.efficios.com

[lttng-dev] [REVIEW REQUEST babeltrace] Provide generic event payload accessors to the Python bindings

[Mathieu Desnoyers]

* J?r?mie Galarneau (jeremie.galarneau at efficios.com) wrote:
> Hi all,
> 
> I'd like to submit a review request for commits 34cf520 .. 3c5bb23 of
> my bindings/python branch[1]. These are patches that were originally
> contributed by Xiaona Han, that I have reviewed and to which I have
> made minor changes (fixed typos and clarified some commit messages).

Some comments:

I notice e.g. in commit
34cf520df2f1df84a74a495a6f9a8d991f168c2e
Return event fields by field name

that the changelog appears to be really wide. In "git show", it goes
beyond a 80-col screen. Please ensure that those changelogs are max 72
lines in length, to make it easier to look at them in git show, and also
to facilitate patch review, where they can easily become indented in
reply messages,

6002606 Remove the unnecessary underscore prefix
looks OK.

fb0936d Support getting the value of enums
looks OK.


fb0ecd1c138a0b903090521406d239cf68b7e5e1
Use a unique name to get a field's value

+                       if id == ctf.type_id.ARRAY:
+                               return self.get_char_array()

I not sure if it is already checked somewhere, but if we have an array
of e.g. uint64_t elements (not characters), is it OK to return a char
array ?

I'd be tempted to return a char array only if the encoding is ASCII or
UTF8, and only if the array contains 8-bit characters. But maybe this is
validated elsewhere ?

5891c4d Support for the sequence type
looks OK

3c5bb23 Add a python bindings sequence test
looks OK

Thanks,

Mathieu




> 
> Thanks,
> J?r?mie
> 
> [1] git://github.com/jgalar/babeltrace.git -b bindings/python
> Web: https://github.com/jgalar/babeltrace/tree/bindings/python
> 
> -- 
> J?r?mie Galarneau
> EfficiOS Inc.
> http://www.efficios.com
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[lttng-dev] [REVIEW REQUEST babeltrace] Provide generic event payload accessors to the Python bindings

[Jérémie Galarneau]

On Mon, Aug 26, 2013 at 11:48 AM, Mathieu Desnoyers
<mathieu.desnoyers at efficios.com> wrote:
> * J?r?mie Galarneau (jeremie.galarneau at efficios.com) wrote:
>> Hi all,
>>
>> I'd like to submit a review request for commits 34cf520 .. 3c5bb23 of
>> my bindings/python branch[1]. These are patches that were originally
>> contributed by Xiaona Han, that I have reviewed and to which I have
>> made minor changes (fixed typos and clarified some commit messages).
>
> Some comments:
>
> I notice e.g. in commit
> 34cf520df2f1df84a74a495a6f9a8d991f168c2e
> Return event fields by field name
>
> that the changelog appears to be really wide. In "git show", it goes
> beyond a 80-col screen. Please ensure that those changelogs are max 72
> lines in length, to make it easier to look at them in git show, and also
> to facilitate patch review, where they can easily become indented in
> reply messages,
>
> 6002606 Remove the unnecessary underscore prefix
> looks OK.
>
> fb0936d Support getting the value of enums
> looks OK.
>
>
> fb0ecd1c138a0b903090521406d239cf68b7e5e1
> Use a unique name to get a field's value
>
> +                       if id == ctf.type_id.ARRAY:
> +                               return self.get_char_array()
>
> I not sure if it is already checked somewhere, but if we have an array
> of e.g. uint64_t elements (not characters), is it OK to return a char
> array ?
>
> I'd be tempted to return a char array only if the encoding is ASCII or
> UTF8, and only if the array contains 8-bit characters. But maybe this is
> validated elsewhere ?
>
> 5891c4d Support for the sequence type
> looks OK
>
> 3c5bb23 Add a python bindings sequence test
> looks OK
>

Thanks for your comments. I have addressed them and rebased the
patches on the latest tree.
The new revisions are 91c9c98 .. f0a440b.

Regards,
J?r?mie

> Thanks,
>
> Mathieu
>
>
>
>
>>
>> Thanks,
>> J?r?mie
>>
>> [1] git://github.com/jgalar/babeltrace.git -b bindings/python
>> Web: https://github.com/jgalar/babeltrace/tree/bindings/python
>>
>> --
>> J?r?mie Galarneau
>> EfficiOS Inc.
>> http://www.efficios.com
>>
>> _______________________________________________
>> lttng-dev mailing list
>> lttng-dev at lists.lttng.org
>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com



-- 
J?r?mie Galarneau
EfficiOS Inc.
http://www.efficios.com