Mirror of the lttng-dev mailing list
 help / color / mirror / Atom feed
From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers)
Subject: [lttng-dev] [REVIEW REQUEST babeltrace] Provide generic event	payload accessors to the Python bindings
Date: Mon, 26 Aug 2013 11:48:55 -0400	[thread overview]
Message-ID: <20130826154855.GA17119@Krystal> (raw)
In-Reply-To: <CA+jJMxvw6sK3GVadKw2jt8mpciwV8Ts-A441wFJiesEbadj0Sg@mail.gmail.com>

* 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



  reply	other threads:[~2013-08-26 15:48 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-26 15:23 Jérémie Galarneau
2013-08-26 15:48 ` Mathieu Desnoyers [this message]
2013-08-27  0:40   ` Jérémie Galarneau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130826154855.GA17119@Krystal \
    --to=mathieu.desnoyers@efficios.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox