[PATCH] osutil: implementation for Win32

[Adrian Buehlmann]

On 10.09.2008 20:51, Matt Mackall wrote:
> On Wed, 2008-09-10 at 15:53 +0200, Adrian Buehlmann wrote:
>> 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?
> 
> Yes.
> 
>> Why is initializing these variables weird?
> 
> a) it's unnecessary
> b) it makes the source uglier
> c) it makes the compiled code bigger
> d) it can hide real uninitialized variable bugs that a compiler may spot
> 
>> 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:
> 
> And that's wrong too. Perhaps I'll fix it. Some time in October at this
> rate.

I see.

> (This is a small example of why code review is important. If we let in
> suboptimal code, it serves as a template for further suboptimal code.
> The UNIX version actually looked worse than the original proposed
> Windows code when it initially went in and I spent several hours beating
> it into shape before running out of steam.)

That's indeed a problem for the learners like myself.

I constantly think the code that is already there must be ok.

>>> 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.
> 
> Yes, I'm lazy. Variables should be declared and the meaningless
> initializers skipped.

Ok. Got that. I didn't understand that there are meaningless initializers.

>>> 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?
> 
> Not only is every variable initialized to an "unused" state, every
> variable has to be set -back- to the "unused" state after use so the
> exit code does the right thing. This is both more complicated and more
> fragile than the preferred style where we 'statically' know which
> variables are in what state based simply on what line of code we're on
> without the extra initialization and re-initialization. Eg: on all the
> lines above between 'goto error_b' and 'error_b:', we know that a and b
> are properly initialized.

Thank you for your explanation and your patience.