[PATCH 4/4] Move the variable initialisations closer to use and add blanks

[Dirk Hohndel]

On Mon, 2013-12-16 at 11:39 -0800, Thiago Macieira wrote:
> On segunda-feira, 16 de dezembro de 2013 10:08:45, Linus Torvalds wrote:
> > I'm no a fan of the "declare variables where they are used" model,
> > because it tends to just encourage people to add crap to functions,
> > and I think it makes it hard to see what the variables in a function
> > are. For the kernel, we have that "if you need more than 5-10 local
> > variables in a function, you're likely doing something wrong" model.
> > Hiding variable declarations tends to just hide problems like that.
> 
> That's a good point. I don't mind having the variables declared at the top 
> provided they are *few* variables. If there are tens of variables and the 
> function is big enough I don't remember what this variable is or does, we have 
> a problem either way. I solved it by moving the variables closer to the use-
> point, but splitting the function is another way.

And generally the one I prefer.
Given my lack of fluency in C++ / Qt I haven't been as strict with this
as I should have... but it's something I plan to push for a little more
strongly again.

> > But hey, it's personal. I much prefer to have local specialized inline
> > functions and putting helper variables in those, for example. I would
> > do that "Save the XML document into a zip file" as a helper function,
> > for example, and *avoid* having that temporary buffer for the filename
> > ("char filename[PATH_MAX];") in the middle of a larger function. And I
> > would feel that *that* would add true readability, and that moving the
> > variables to be mixed in with the code is actually a bad move.
> 
> Hmm... that's actually a cleaner solution, since right now we transfer a file 
> name, which must then be reopened and deleted. It would be better if we could 
> transfer a file handle of some sort.
> 
> The problem is that we can't do it with libzip: we can't give it a FILE* or 
> file descriptor to write to, as zip_fdopen can only open in read-only mode. So 
> we need to give it a temporary file name, which is insecure, and delete it 
> later.
> 
> Still, it would make the thing a little cleaner if the file name were passed by 
> the caller function. I can make that change easily. I can also modify the code 
> to make it closer to your request. But I can't promise to always do it like 
> that... muscle memory will kick in every now and then.

And then I just need to do my job as maintainer and complain :-)

> > So I feel - pretty strongly - that the whole C++ "mix code and
> > declarations" model is crap, and largely brought on by bad coding
> > styles that think that long functions that do several different things
> > with lots of variables are somehow acceptable.
> 
> There are two other reasons to doing that way:
> 
> 1) classes with constructors have a non-zero cost of initialisation. For the 
> value-type Qt classes like QByteArray, QList and QString, the cost is very 
> small: one atomic increment (Qt 4) or two comparisons (Qt 5).
> 
> 2) for non-Qt classes, they may throw, so you don't want to initialise them 
> until you actually need them.
> 
> C++ also often subscribes to the Resource Allocation Is Initialisation 
> principle (RAII). I was going to do that for some of the code in my refactor, 
> but thought better of it and just used gotos.

As Linus pointed out, it's ok to declare the variables later in the
function. For example if you have a bunch of sanity checks in the
beginning (dive != NULL, amount_selected == 1, waxing phase of the moon)
then create a scope after those are done and declare the variables in
that scope

> > Of course, having seen how painful it can be to just add a small
> > random helper function (because you need to declare it in another file
> > etc), I can see _why_ C++ people seem to do it. I still think it's a
> > sign of a disease, but I've grumbled enough about it. In the end, I
> > don't tend to touch UI code in C++, so why should you care what I
> > think?
> 
> Well, there's an ongoing discussion right now for C++17 to be able to add 
> random small helpers in the same class context without declaring it in the 
> class header. If you hadn't mentioned this before, I would probably have 
> ignored the thread when it first came up.
> 
> So how do you feel about having (indirectly) contributed to the C++ standard? 
> :-)

Oh no. Now you made him weep in pain...

/D