Page MenuHomePhabricator

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

Repository
rL LLVM

Event Timeline

roxma created this revision.Jan 7 2019, 11:57 PM
bindings/python/clang/cindex.py
2998 ↗(On Diff #180620)

Why did you remove the FIXME comment?

bindings/python/clang/cindex.py
2998 ↗(On Diff #180620)

@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 ↗(On Diff #180620)

@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 ↗(On Diff #180620)

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 ↗(On Diff #180620)

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.

Closed by commit rL352039: Fix python3 compability issue in clang binding (authored by serge_sans_paille, committed by ). · Explain WhyJan 24 2019, 2:34 AM
This revision was automatically updated to reflect the committed changes.

Thanks @roxma for the patch!