Skip to content

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 12, 2018

The char_converter converter can be simpler.

The current code doesn't work correctly with characters from 0x0e to 0x1f and >= 0x7f. And it escapes ? for unknown reasons.

https://bugs.python.org/issue20180

@taleinat
Copy link
Contributor

@serhiy-storchaka, I wrote the existing escaping mechanism based on my reading of the C standard, since self.c_default needs to be a valid C character literal. It seemed to be a safer way to ensure creating a valid C char literal than relying on e.g. repr(bytes(...)), which we might decide to change in the future.

I recall also being surprised by '?' having an escape sequence, but I don't remember why I decided to keep it at the end; perhaps just for completeness, having included all of the other escape sequences.

I definitely defer to your judgement on this. I'm not a C expert and so I was extra-careful when I wrote this code, perhaps overly so.

@serhiy-storchaka
Copy link
Member Author

I think that the repr of str and bytes was intentionally made mostly compatible with C (except that they can use either singular or double quotes). It is very unlikely that they will be made incompatible in future.

? can be need to be escaped because of trigraph sequences like ??=. But in our case ? is a single character.

@taleinat
Copy link
Contributor

taleinat commented Oct 12, 2018

>>> repr(b'\0')
"b'\\x00'"

Assuming '\x00' is a valid C char literal, it's not as nice as '\0'. IMO not a deal-breaker, for your consideration.

Same goes for '\1' etc.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you fixed a bug, so I would expect a new test to check for non-regression. Is it something doable? Sorry, I didn't look at test_clinic.py.

@serhiy-storchaka serhiy-storchaka merged commit 65ce60a into python:master Dec 25, 2018
@serhiy-storchaka serhiy-storchaka deleted the clinic-char_converter branch December 25, 2018 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants