[PATCH] osutil: implementation for Win32

[Adrian Buehlmann]

On 09.09.2008 21:51, Matt Mackall wrote:
> On Tue, 2008-09-09 at 20:51 +0200, Adrian Buehlmann wrote:
>> # HG changeset patch
>> # User Petr Kodl <petrkodl at gmail.com>
>> # Date 1220959680 14400
>> # Node ID 020fe75b47f569a29c25f8a5fef7451100d72772
>> # Parent  9141bebefe3ed0498ff07271214bfca8b4eb0d27
>> osutil: implementation for Win32
> 
> Thanks, guys. This is looking much better. A few more comments, I'm
> afraid.
> 
>> +static PyObject *listdir(PyObject *self, PyObject *args, PyObject *kwargs)
>> +{
>> +	PyObject *statobj = NULL;
>> +	PyObject *list = NULL;
>> +	PyObject *items = NULL;
>> +	PyObject *ctor_args = NULL;
>> +	PyObject *item0 = NULL;
>> +	PyObject *item1 = NULL;
>> +	PyObject *py_st = NULL;
> 
> There's a slightly weird thing going on here with initializing
> variables.

Does that mean those variables should not be initialized?

Why is initializing these variables weird?

Or are you referring to some specific of these variables?

It seems to me the preexisting (unix) listdir implementation does something
very similar with variable initializations:

'''
static PyObject *listdir(PyObject *self, PyObject *args, PyObject *kwargs)
{
	static char *kwlist[] = { "path", "stat", NULL };
	DIR *dir = NULL;
	PyObject *statobj = NULL;
	PyObject *list = NULL;
	PyObject *err = NULL;
	PyObject *ctor_args = NULL;
	char *path;
	char full_path[PATH_MAX + 10];
	int path_len;
	int need_stat, keep_stat;
	int dfd;
...
'''

Is there a problem initializing these pointers to NULL too?

Or what is weird there (if it is)?

> I eventually figured out it's because of a misunderstanding
> of how the 'goto' error-handling style works. It's supposed to work
> roughly like this:
> 
> 	a = malloc(x);
> 	ret = NO_MEMORY;
> 	if (!a)
> 		goto error_a;
> 
> 	b = open(y);
> 	ret = OPEN_FAILED;
> 	if (!b)
> 		goto error_b;
> 
> 	 ret = OOPS;
> 	do { 
> 		if (!trysomething(a,b))
> 			goto error;
> 		...
> 	}
> 	ret = SUCCESS;
> 
> error: /* need to free all resources */
> 	close(b);
> error_b: /* don't need to close b, because we didn't open it */
> 	free(a);
> error_a: /* don't need to free a */
> 	return ret;
> 
> The idea is that you can bail out easily anywhere in your function even
> if there are already some resources allocated that need cleaning up and
> not have the error handling paths dominate your code or cause massive
> indenting. 

You have omitted the declaration of variables.

Shouldn't variable 'a' be declared at the beginning of the function
like this?

  sometype *a = NULL;

or is the initialization the weird thing and it should be just

  sometype *a;

?

> To make this work, you don't have to do anything special with variable
> initialization

What has been done special about variable initialization in the patch?

> (and you don't have to unset things at the end, either!),
> you just have to a entry point in the 'unrolling' for every 'block' of
> resources you allocate.
> 
>> +	if (INVALID_HANDLE_VALUE == fh) {
> 
> I'm not a fan of this backwards comparison style, btw.
> 
>> +		int isro  = fd.dwFileAttributes & FILE_ATTRIBUTE_READONLY;
> 
> Read-only isn't meaningful to Mercurial.
> 
>> +		if (isdir && (!strcmp(fd.cFileName, ".")
>> +						|| !strcmp(fd.cFileName, "..")))
>> +			continue;
>> +
>> +		items = PyTuple_New(keepstat ? 3 : 2);
>> +
>> +		item0 = PyString_FromString(fd.cFileName);
>> +		item1 = PyInt_FromLong(isdir ? _S_IFDIR : _S_IFREG);
>> +
>> +		if (!items || !item0 || !item1) {
>> +			PyErr_NoMemory();
>> +			goto error;
>> +		}
>> +
>> +		PyTuple_SetItem(items, 0, item0);
>> +		PyTuple_SetItem(items, 1, item1);
>> +		item0 = item1 = NULL;
>> +
> 
> I know this was probably cut-and-pasted from the original, but this
> would probably be better with something like:
> 
> if (keepstat) {
> 	...
> 	items = Py_BuildValue("siO", fd.cFileName, mode, py_st);
> } else 
> 	items = Py_BuildValue("si", fd.cFileName, mode);
> 
> if (!items)
> 	...
> 
> That is, we can get rid of a bunch of single object creates, item sets, 
> and error checking.
>