<a href="https://github.com/torvalds/linux/blob/d158fc7f36a25e19791d25a55da5623399a2644f/fs/ext4/resize.c#L652" rel="nofollow">https://github.com/torvalds/linux/blob/d158fc7f36a25e19791d2...</a><p>Variables `three`, `five` and `seven` are better described as `next_power_of_three`, `next_power_of_five` and `next_power_of_seven`. Since the `ext4_list_backups` function should iterate through 1 (= 3^0 = 5^0 = 7^0), 3, 5, 7, 3^2, 5^2, 3^3, 7^2, ... and 1 should not repeat three times, the initial value of `next_power_of_three` (or any of others) should be 1 and those of others should not be 1. The naming is a bit unfortunate (couldn't they be `threes` etc., for example?) but actually makes sense.
Read the comment above ext4_list_backups right above:<p><a href="https://github.com/torvalds/linux/blob/d158fc7f36a25e19791d25a55da5623399a2644f/fs/ext4/resize.c#L652" rel="nofollow">https://github.com/torvalds/linux/blob/d158fc7f36a25e19791d2...</a><p><pre><code> /*
* Iterate through the groups which hold BACKUP superblock/GDT copies in an
* ext4 filesystem. The counters should be initialized to 1, 5, and 7 before
* calling this for the first time. In a sparse filesystem it will be the
* sequence of powers of 3, 5, and 7: 1, 3, 5, 7, 9, 25, 27, 49, 81, ...
* For a non-sparse filesystem it will be every group: 1, 2, 3, 4, ...
*/
</code></pre>
I'm not sure what's so noteworthy about that. There's ton of arcane code in any kernel, as long as it's commented correctly there's no issue.
Lot's of people suggesting to add explanatory comments. I don't think that's a good idea, it would be better to change the names of the variables:<p><pre><code> unsigned counter1 = 1;
unsigned counter2 = 5;
unsigned counter3 = 7;
</code></pre>
or use an array:<p><pre><code> unsigned counter[3] = {1,5,7};
</code></pre>
No more confusion. That being said, I doubt that the names of these local variables is actually a real source of confusion and is not a problem that needs to be fixed.
That's a very... awkward of doing quite a simple thing, generating ascending interleaved powers. I'd probably use one or more arrays of multipliers to maintain the counters. Simpler and more general -- not often that you can get a solution with both these two attributes.
And here's how the "rainbows and butterflies" world of "code perfectionists" come crashing down.<p>Yes, the naming is unfortunate. No, there's probably no way to make it better (and if you think code reviews suck, wait until you see a kernel code review) (Well, this came directly from Linus but there's usually some discussion as well)<p>But in the end this has shipped and is running. (Well, the double goto from Apple as well, but I doubt that went through a code review)
there are many, many problems in this code style which lead to this kind of problem.<p>localised comments may help but naming things properly would help more (and negate a comment being required at all), actually adopting some better practices would be a better fix going forward.<p>looking at the called function's comment:<p>The counters should be initialized to 1, 5, and 7 before
* calling this for the first time.<p>so how about making a function with the same name, ending in '_first' (maybe rename the original _next) which initialises these variables internally before calling the version intended to be called multiple times...?<p>since the parameters are not well described by their names renaming them would help too... it is not a pointer to a 3 a 5 or a 7. it might not even be pointing at multiples... but current_multiple_of_three etc. would be better. there is some reason why these values are significant in this algorithm (which I do not know) and the best names would capture that.<p>the code is moderately difficult to read in the function itself - the method of swapping around a local copy of a pointer based on the conditions is not very easy to follow. although makes sense here to avoid excessive code... comments would help make it clearer what is going on.
Seems like a perfectly reasonable naming scheme to me. Anyone foolish enough to think that the variables are there to hold the values 3, 5 and 7 is probably wasting their time on the code in the first place.<p>Besides, it made you read the code and the comments.
Reminds me of 'how to write unmaintainable code':<p><a href="http://mindprod.com/jgloss/unmaincamouflage.html" rel="nofollow">http://mindprod.com/jgloss/unmaincamouflage.html</a>