Why did you make this change? Returning unicode in Py27 where it was previously str is an API-incompatible change.

I would make this and if { } else if {} else {} block instead of nested if

I agree it looks a bit inelegant, and I'm not the most elegant programmer, so no offense if you or someone else changes it. But, you need an outer if to check for a string to send to the string translators below, and then you need to decide which string decoder you want.

I am not sure it would make this any more elegant as we would have either to duplicate the following logic (string value comparison) or refactor it in a function. To me this looks fine.

Yep. You're both right. I only looked at the diff preview and was missing the extra context.

I am not sure it would make this any more elegant as we would have either to duplicate the following logic (string value comparison) or refactor it in a function. To me this looks fine.

Why not use PyUnicode_AsUTF8 as we do on Python 3 ?

Yeah. PyUnicode_AsASCIIString also returns a new object, so this line will cause a memory leak.
https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_AsASCIIString

should be return false;

This block should probably use the UTF8 encoding just like we do for py3. You can make use of a smart pointer so you don't need to manage the ref count manually. I would remove the PyObject* ascii_str; declaration, and use a block like this:

          PythonHelpers::PyObjectPtr py_str( PyUnicode_AsUTF8String( value ) );
          if( !py_str )
              return false;
          str = PyString_AS_STRING( py_str.get() );

This variable declaration does not need to be hoisted.

this line did not need to be changed.

This should be changed to std::string str;

This is not needed. See longer comment below.

This block can be done the same way as above with a smart pointer.

in py2, folks can very often accidentally use a mixture of Unicode and plain strings. It would be best if that was allowed.

Ideally, you convert all py2 strings into unicode at the calling boundary, and then expect everything to be unicode internally.

I haven't dug into your C++ code (why in the world are you hand-writing the bindings??? -- Cython would make this a lot easier) -- but a little utility conversion function should be fairly easy to write and drop in everywhere a string is expected.

It looks like this is testing for non-ascii character -- but if it's allowed in py3, why not py2??

I haven't dug into your C++ code (why in the world are you hand-writing the bindings??? -- Cython would make this a lot easier)

It really doesn't, and this sort of comment is not helpful.

no, it's not -- sorry.

But in any case, I have now looked at the C++ and it looks like you want a utf-8 encoded std::string from a python string.

In python, you would do this by calling .encode('utf-8') on the object, and that would work on py2 strings and unicode objects, and py3 strings. So maybe you could call the python from C++, but that's awkward, and maybe slow, so you could instead call:

PyString_Encode on py2 strings, and

PyUnicode_AsUTF8String on py2 unicode objects and py3 strings.

In any case, why require ascii on py2 and allow full unicode on py3?

Sorry my mistake. I chose to stay close to the original contributor implementation. I will fix this.

Since this change causes the function to possible return early after the python variable object has been allocated, we need to use a smart pointer to guard the lifetime of that object. Here's an updated version of this function which does that:

static PyObject*
Variable_new( PyTypeObject* type, PyObject* args, PyObject* kwargs )
{
	static const char *kwlist[] = { "name", "context", 0 };
	PyObject* context = 0;
	PyObject* name = 0;

	if( !PyArg_ParseTupleAndKeywords(
		args, kwargs, "|OO:__new__", const_cast<char**>( kwlist ),
		&name, &context ) )
		return 0;

	PyObjectPtr pyvar( PyType_GenericNew( type, args, kwargs ) );
	if( !pyvar )
		return 0;

	Variable* self = reinterpret_cast<Variable*>( pyvar.get() );
	self->context = xnewref( context );

	if( name != 0 )
	{
#if PY_MAJOR_VERSION >= 3
		if( !PyUnicode_Check( name ) )
			return py_expected_type_fail( name, "unicode" );
#else
		if( !( PyString_Check( name ) | PyUnicode_Check( name ) ) )
		{
			return py_expected_type_fail( name, "str or unicode" );
		}
#endif
		std::string c_name;
		if( !convert_pystr_to_str(name, c_name) )
			return 0;
		new( &self->variable ) kiwi::Variable( c_name );
	}
	else
	{
		new( &self->variable ) kiwi::Variable();
	}

	return pyvar.release();
}

Thanks for catching this.