[lifthrasiir]

https://github.com/torvalds/linux/blob/d158fc7f36a25e19791d2...

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.

[Syssiphus]

A perfect example of a missing code-comment.

[dangrossman]

The explanatory comment is 40 lines earlier in the same file where the function those variables are being passed to is defined. It need not be repeated every couple lines; "being clear to outsiders linked to a specific line of a specific file without context" is not a reasonable concern.

[annnnd]

In this case I think it is not about comment at all - it is about poor naming. More descriptive (or less misleading) var names would help in this case.

[spinlock]

I agree. 'power_of_three' isn't really that much of a hardship to type and it obviates the need for a comment.

[orclev]

The function declared earlier is perfectly fine, the comment is sufficient to explain what it's doing, but functions should be understandable simply by reading their source and in that respect the linked function fails. This would be really simple to solve with a simple comment, E.G.

  // current power of three, init to 3^0
  unsigned three = 1;

The fact that this looks like a bug at first glance is a pretty good indication that there should be some explanation of what exactly it's doing.

[awda]

This is actually contrary to Linux kernel "good style". Functions should be short and understandable. Comments should be on the top of the function, describing what the function is for. Comments describing variables in functions are discouraged.

[jrowen]

Are you supporting that dogma or just stating it?

This brings to mind the Orwell essay on language usage[1]: Break any of these rules sooner than say anything outright barbarous.

IMO having "three = 1" with no immediate context qualifies as barbarous. Yes it would be best to rename the variable, but, failing that, just toss in a freaking comment for common sense's sake.

[1]https://www.mtholyoke.edu/acad/intrel/orwell46.htm

[fnordfnordfnord]

>"Also, try to avoid putting comments inside a function body: if the function is so complex that you need to separately comment parts of it, you should probably go back to chapter 4 for a while. You can make small comments to note or warn about something particularly clever (or ugly), but try to avoid excess. Instead, put the comments at the head of the function, telling people what it does, and possibly WHY it does it."

The code example being discussed has comments sprinkled throughout the functions, and the "good style" / standard also says try to avoid not avoid at all cost. Also, something about blind adherence to rulebooks.

https://www.kernel.org/doc/Documentation/CodingStyle

[orclev]

As a general rule that's probably fine, but in cases where a variable is confusing either a better name or a explanatory comment is probably appropriate. Ideally you'd pick a name that fully captures the intent of the variable, but in this case it's rather vague, and a name that isn't would be rather unwieldy, so a comment is a nice compromise. The alternative would be to call it something like:

    unsigned current_power_of_three = 1;

or something equally verbose. I'd expect such silliness in some enterprisey java code, but I think most people would agree a short explanatory comment is probably the superior choice.

[ecesena]

It's C :) edit: It's Linux kernel :)

    /* current power of three, init to 3^0 */

[spiralpolitik]

// became valid C in C99. 15 years ago.

[ecesena]

Sorry, I should have said it's Linux kernel [1].

    Linux style for comments is the C89 "/* ... */" style.
    Don't use C99-style "// ..." comments.

[1] https://www.kernel.org/doc/Documentation/CodingStyle

[Navarr]

But it is! God forbid the original function disappear or change completely and the function below it loses all context and description.

[mattmanser]

More like a terribly named variable.

[denizozger]

Agreed, comments are generally a way to compensate failure to express ourselves in the code (in this case bad naming).

[thaumasiotes]

Actually, there's a difficulty here that naming can't solve; I don't see a better method than the comment.

The goal is to enumerate the powers of 3, 5, and 7, once each. Since power sequences all overlap at x^0 = 1, but we specifically don't want to enumerate 1 three times, we have to give one (or, from an alternative viewpoint, two) of the variables special treatment. Whether you name the variables "three", "five", and "seven", or "next_power_of_three", "next_power_of_five", and "next_power_of_seven", you're doing something strange by starting one of them at 1 and the other two past 1, and that should be commented on. The naming-only solution "powers_of_three_initialized_starting_at_three_to_the_zeroeth", "powers_of_five_initialized_starting_at_five_to_the_first", and "powers_of_seven_initialized_starting_at_seven_to_the_first", is hilariously awful, and still requires a comment to explain why the threes variable is more (or less) special than the other two.

[stephencanon]

The goal is not to enumerate the powers of 3, 5, and 7. The goal is to iterate through the groups which hold BACKUP superblock/GDT copies. “three”, “five” and “seven” are not especially good names for iterator state, nor is there an obvious good reason for exposing the inner details of iterator state.

[chengiz]

Yes thank you! People seem to be missing this point. The variables should be simply named something like group_counter_a, group_counter_b, group_counter_c, with an explanation of why their default values are what they are.

[userbinator]

Or how about just combining all those variables into an array named "iterator_powers" or similar? There could be another one, "iterator_multipliers" containing 3, 5, and 7. I don't know if this is actually a "best practice" or rule, but I've found that if I want to name variables after numbers, usually what I'm trying to do should be using an array.

[mseebach]

I think the problem is the mixing of concerns: The generation of the sequence (which apparently is a function of whether the filesystem is sparse or not) and whatever it is trying to verify.

[CrystalCuckoo]

Not really, the problem lies in its naming. Comments that explain what the variables represent signify that they should have been better named in the first place to avoid any confusion.