* Jérémie Galarneau ([email protected]) 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 > [email protected] > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
