This is an archive of the discontinued LLVM Phabricator instance.

fix python3 compability issue
ClosedPublic

Authored by roxma on Jan 7 2019, 11:57 PM.

Details

Summary

The file contents could be of str type. Should use byte length instead
of str length, otherwise utf-8 encoded files may not get properly parsed
for completion.

Source issue: https://github.com/ncm2/ncm2-pyclang#2

Diff Detail

Event Timeline

roxma created this revision.Jan 7 2019, 11:57 PM
bindings/python/clang/cindex.py
2998

Why did you remove the FIXME comment?

bindings/python/clang/cindex.py
2998

@roxma LGTM except this FIXME removal.

roxma marked an inline comment as done.Jan 14 2019, 7:30 PM
roxma added inline comments.
bindings/python/clang/cindex.py
2998

@serge-sans-paille

the contents.read() looks almost the same as line 2817.

It is better to keep the code consistent. The FIXME doesn't seem to be helpful.

jstasiak marked an inline comment as done.Jan 15 2019, 2:15 AM
jstasiak added a subscriber: jstasiak.
jstasiak added inline comments.
bindings/python/clang/cindex.py
3001

Nitpicking: the description only mentions changes related to file contents but this modification (fspath(name) -> b(fspath(name))) likely fixes a different issue, it may be worth mentioning this in the commit message. Unless this extra b() call is not strictly necessary here but added for consistency?

serge-sans-paille added inline comments.
bindings/python/clang/cindex.py
2998

ok, I'll trust you on this.

This revision is now accepted and ready to land.Jan 16 2019, 1:57 AM

@roxma: do you want me to commit this on your behalf?

roxma added a comment.Jan 22 2019, 5:42 AM

Yes please.

This revision was automatically updated to reflect the committed changes.

Thanks @roxma for the patch!