5 ms·

705 /* 706 * Iterate through the groups which hold BACKUP superblock/GDT copies in an 707 * ext4 filesystem. The counters should be initialized to 1, 5,

705 /* 706 * Iterate through the groups which hold BACKUP superblock/GDT copies in an 707 * ext4 filesystem. The counters should be initialized to 1, 5, and 7 before 708 * calling this for the first time. In a sparse filesystem it will be the 709 * sequence of powers of 3, 5, and 7: 1, 3, 5, 7, 9, 25, 27, 49, 81, ... 710 * For a non-sparse filesystem it will be every group: 1, 2, 3, 4, ... 711 */ 712 static unsigned ext4_list_backups(struct super_block *sb, unsigned *three, 713 unsigned *five, unsigned *seven)

hinoki 6y ago
Yeah, maybe calling them pow3, pow5, pow7 would be a bit clearer? But a 10s search for a comment cleared it up, so I don’t really see the problem...
- coinerone 6y ago
  The sound of Linus typing in all caps intensifies
- regularfry 6y ago
  The problem is not that it isn't clear. The problem is that it is extremely clear in a way that makes you confidently think something totally wrong.
  - chasd00 6y ago
    That situation is one of the most costly mistakes, in every way i can think of, in software engineering period.
- magicalhippo 6y ago
  IMHO a better approach would be to make a helper function that initializes those variables, rather than relying on all callers to do it every time. Then you could put the relevant comment about the weirdness of "three = 1" there, and callers wouldn't have to remember the magic sequence.
- ronyeh 6y ago
  I see a problem: https://github.com/torvalds/linux/blob/master/fs/ext4/resize.c#L750-L752 https://github.com/torvalds/linux/blob/master/fs/ext4/resize... Upon a quick code review, these lines look buggy: unsigned three = 1; unsigned five = 5; unsigned seven = 7; Without digging deeper, the reader of this code thinks "The first line surely must be a bug, right???"
  - scotty79 6y ago
    Not really. Nobody sane would think other sane developer with any expeirience would call a simple variable according to what it initially contains. You'd expect either name related to what is the meaning of the contents, or meaningless name.
    - oji0hub 6y ago
      People make typos and simple mistakes all the time, regardless of experience.
      - Guthur 6y ago
        +1 seen it so many times, often times from some cut and paste heavy refactor, one is more or less going on autopilot just moving substrings around and then bam. Review process hopefully catches it, when it is a bug, and potentially when the semantics are unclear...
      - scotty79 6y ago
        True. But the intention of writing int three = 3; is insane. I would never ascribe to previous programmer (unless he just learned what variables are) the intention of creating variable (not even constant, which wouldn't make it any better) named 'three' and assigning actual value 3 to it.
        forgottenlogin9 6y ago
        If that is insane, you may not have issue with this specific line but you surely have issue with the 2 “insane” lines that follow it?
- croes 6y ago
  Pow3, pow5, pow7 still sounds wrong without explanation why pow3 starts at 3^0 but pow5 and pow7 start at 5^1 and 7^1
- calibas 6y ago
  And the scope for "three" is quite small, a single short function. It's not like it's using a global variable or #define.
- Iwan-Zotow 6y ago
  it is IRRATIONAL code (maybe rational within context, but ...)
- akimball 6y ago
  threes_cursor fives_cursor sevens_cursor

hinoki 6y ago

Yeah, maybe calling them pow3, pow5, pow7 would be a bit clearer? But a 10s search for a comment cleared it up, so I don’t really see the problem...

coinerone 6y ago
The sound of Linus typing in all caps intensifies
regularfry 6y ago
The problem is not that it isn't clear. The problem is that it is extremely clear in a way that makes you confidently think something totally wrong.
- chasd00 6y ago
  That situation is one of the most costly mistakes, in every way i can think of, in software engineering period.
magicalhippo 6y ago
IMHO a better approach would be to make a helper function that initializes those variables, rather than relying on all callers to do it every time. Then you could put the relevant comment about the weirdness of "three = 1" there, and callers wouldn't have to remember the magic sequence.
ronyeh 6y ago
I see a problem: https://github.com/torvalds/linux/blob/master/fs/ext4/resize.c#L750-L752 https://github.com/torvalds/linux/blob/master/fs/ext4/resize... Upon a quick code review, these lines look buggy: unsigned three = 1; unsigned five = 5; unsigned seven = 7; Without digging deeper, the reader of this code thinks "The first line surely must be a bug, right???"
- scotty79 6y ago
  Not really. Nobody sane would think other sane developer with any expeirience would call a simple variable according to what it initially contains. You'd expect either name related to what is the meaning of the contents, or meaningless name.
  - oji0hub 6y ago
    People make typos and simple mistakes all the time, regardless of experience.
    - Guthur 6y ago
      +1 seen it so many times, often times from some cut and paste heavy refactor, one is more or less going on autopilot just moving substrings around and then bam. Review process hopefully catches it, when it is a bug, and potentially when the semantics are unclear...
    - scotty79 6y ago
      True. But the intention of writing int three = 3; is insane. I would never ascribe to previous programmer (unless he just learned what variables are) the intention of creating variable (not even constant, which wouldn't make it any better) named 'three' and assigning actual value 3 to it.
      - forgottenlogin9 6y ago
        If that is insane, you may not have issue with this specific line but you surely have issue with the 2 “insane” lines that follow it?
croes 6y ago
Pow3, pow5, pow7 still sounds wrong without explanation why pow3 starts at 3^0 but pow5 and pow7 start at 5^1 and 7^1
calibas 6y ago
And the scope for "three" is quite small, a single short function. It's not like it's using a global variable or #define.
Iwan-Zotow 6y ago
it is IRRATIONAL code (maybe rational within context, but ...)
akimball 6y ago
threes_cursor fives_cursor sevens_cursor