mgorny added a comment.
A few comments/questions. However, please note that those are generic Python
comments and I haven't used or tested the clang Python API yet.
================
Comment at: bindings/python/clang/cindex.py:77
+# Python 3 strings are unicode, translate them to/from utf8 for C-interop
+if type(u"") == str:
+ class c_string_p(c_char_p):
----------------
What is the Python3 version you're aiming to support? If I recall correctly,
`u""` is allowed (again) since 3.4. And even then, the condition looks weird
and makes me think a while before I figure out what it's supposed to mean.
================
Comment at: bindings/python/clang/cindex.py:518
- for i in xrange(0, count):
+ for i in range(0, count):
token = Token()
----------------
compnerd wrote:
> IIRC, `range` and `xrange` did have some performance difference. This would
> slow down the bindings on python 2. The difference is obviously not
> immediately visible unless count is very high. I wonder if we can do
> anything here to detect the python version and dispatch to `xrange` in python
> 2.
The difference is that `range()` used to construct a complete list in Python 2.
In Python 3, `xrange()` (that uses iterator) got renamed to `range()`.
If you want to avoid performance impact (not sure if it's really measurable
here), you can do something alike C for loop:
i = 0
while i < count:
#...
i += 1
It's not really idiomatic Python though. OTOH, it won't take more lines than
the conditional.
================
Comment at: bindings/python/clang/cindex.py:623
"""Return all CursorKind enumeration instances."""
- return filter(None, CursorKind._kinds)
+ return [x for x in CursorKind._kinds if x]
----------------
Why are you changing this? The old version seems to be correct for Python3.
================
Comment at: bindings/python/clang/cindex.py:2371
+
def __repr__(self):
----------------
Seems to be mistakenly added.
================
Comment at: bindings/python/clang/cindex.py:2573
if len(args) > 0:
- args_array = (c_char_p * len(args))(* args)
+ args_array = (c_string_p * len(args))()
+ for i,a in enumerate(args):
----------------
I may be wrong but I think you could use a list comprehension here.
args_array = (c_string_p * len(args))([c_string_p(x) for x in args])
You can also try without `[]` to avoid the overhead of constructing list, if
the function can take an iterator.
================
Comment at: bindings/python/tests/cindex/test_translation_unit.py:62
def test_unsaved_files_2():
- import StringIO
+ try:
+ from StringIO import StringIO
----------------
Could you try inverting this? Python 2.7 already has `io.StringIO`, so that
branch is much more likely to work.
https://reviews.llvm.org/D26082
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
