Submitting code (was Re: [PATCH] auto rename...)

[Dirkjan Ochtman]

Herbert Griebel <herbertg <at> gmx.at> writes:
> - There are currently 5 bugs in the addremove feature (4 minor, 1 major,
>   not counting the speed problem), this makes 5 patches.
> - For the speed and name matching, -- it's complex --, let's say it's again
>   5 larger and not-so-easy-to-understand patches.

Assuming this would be just about the same patch size in total, I'd say
reviewing 10 separate patches would be *much* easier than reviewing the grand
total of your changes. Actually your patch size was the reason I haven't been
enable to review it, it's just to much to sort out. I should have mentioned
that, maybe, but I wasn't sure whether we had any guidelines on it before.

> If this "obscure feature" does not need this and no one is willing to go
> through this, I will stop right now and scrap it all, but I would have
> wanted to know that a little earlier before annoying anybody. So there
> is a decision to be made here.

Well, for my use at least, automatic renaming doesn't come up very often.
Problem is, this is exactly the kind of thing that "expert" users wouldn't do
very often (because they more easily remember to add/remove/move themselves),
but which is a great addition for newer users who are not yet used to VCS.

> The local functions share the scope of the parent function,
> so you have less clutter than with "self" when using classes, or
> when passing-on all variables per arguments. If you have never
> programmed in C++ you may have trouble with that style.
> Changing variables in the inner scope only works for mutable
> types without defining it locally. I don't know if this is
> against the coding guidelines.

I often dislike having to read a function that contains lots of little local
functions, because the dependency of variables isn't clear. I think most people
read code linearly (certainly in the first pass), meaning that it gets confusing
if, for example, you define a function first and define the variables used in
that function later. Also, it makes it harder to see what variables an inner
function depends on, because I now have to read the whole function body instead
of just reading the function signature. I'm not sure what Matt thinks of this
(and there are plenty of local functions in the codebase), but for this reason I
think local functions should be avoided, certainly for things that are only done
once ("naming a code block" pattern -- just use comments for this) but also for
other things that don't depend on a lot of local state (if they do, a local
function may make sense, as otherwise you'd just be putting all of the local
state in the function signature).

So, I for one would like to get this integrated, but it would go a long ways
towards integration if you could split it up in a progression of smaller patches.

Cheers,

Dirkjan