Page 2 of 2

Re: FreeMem()

Posted: Thu Dec 16, 2010 8:28 am
by wololo
it's about the use of unnecessary variables and code.
When code is shorter, we have less bugs.

Moskito's suggestion adds an extra variable that is unnecessary given how sizeof is used by the compiler. It doesn't make the code any more readable, hence my "joke" about doing it with 4 additional variables for the fun.

Of course I'm only arguing about this because being wrong is one thing, but adding on top of that that the correct code is "ugly" is just gratuitous. I'm the first one to admit when my code is incorrect, but this time, it's the best possible solution, so I don't like being insulted for doing the right thing :evil:

Re: FreeMem()

Posted: Thu Dec 16, 2010 9:22 am
by m0skit0
I don't quite understand your point, wololo. I don't understand your supposed examples either because they make no sense. I can also make your code make look like nonsense, look:

Code: Select all

for (i = 0; i < sizeof(memids)/sizeof(u32)++; i)
Unfortunate typo. Makes sense? No it doesn't. I still don't get your point. Do you want to make me look bad or ridiculize me? Because that's what it seems. We're here to discuss the best way to do it, not put nonsense to make own code look better.
Wololo wrote:When code is shorter, we have less bugs.
When code is harder to read, we have more bugs and don't find them. I can code that for loop shorter:

Code: Select all

for (i = 0; i < sizeof(memids)/4; ++i)
Is this better? Of course it's not.
Wololo wrote:It doesn't make the code any more readable
If you mean that

Code: Select all

int num_memids = sizeof(memids)/sizeof(u32);
for (i = 0; i < num_memids; ++i)
is not more readable than

Code: Select all

for(i = 0; i < sizeof(memids)/sizeof(u32); i++)
then I have nothing more to talk about. Really, you're approaching nonsense here.
Wololo wrote:I'm the first one to admit when my code is incorrect, but this time, it's the best possible solution, so I don't like being insulted for doing the right thing :evil:
I don't want to argue over the "I'm the first one to admit when my code is incorrect" thing, but from that to say you've been insulted, I think you're going toooo far fetched. That code is ugly and less readable, like it or not it's a fact. Also I said I'm not going to discuss about this because it's a very very minor issue, just a side comment, but you jumped on it like it's the most important thing we're discussing here.

I'm done with this useless discussion, code it as you want.

Re: FreeMem()

Posted: Thu Dec 16, 2010 9:55 pm
by wololo
m0skit0 wrote: If you mean that

Code: Select all

int num_memids = sizeof(memids)/sizeof(u32);
for (i = 0; i < num_memids; ++i)
is not more readable than

Code: Select all

for(i = 0; i < sizeof(memids)/sizeof(u32); i++)
then I have nothing more to talk about.
That's close enough to what I mean, yes (except I'd use ++i instead of i++ in my version too).

In my experience, when I see such a (sizeof(A) / sizeof(B) ) in a loop it screams to me "loop on all the elements of a static array". I'm actually more used to sizeof(A)/sizeof(A[0]) but there's a risk if the array is empty <-- which is rare for a static array, really.
But because this is such as frequent use of size of + division, yes, I think it's more readable, but probably just a matter of getting used to it, I guess.

so how about:

Code: Select all

for(i = 0; i < sizeof(memids)/sizeof(memids[0]); ++i)
I truly think that's very beautiful, readable, and doesn't need an intermediate variable.
UNLESS we need to reuse that value later on, of course


You also said that the division was made at every step of the loop, that was your main argument. And that was incorrect. On top of that in order to "fix" this problem that didn't exist, you suggest to introduce an additional variable (not even a constant), which to me increases the risk of bugs / side effects. <-- this is my main concern, rather than readability. There's a reason functional languages minimize the number of variables.

Let's agree that we disagree, then.
I also apologize for getting off topic on this one :oops: