11 ms·
`three = 1` in the Linux sourcecode (2014)
- post-factum 6y ago705 /* 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 agoYeah, 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 agoThe sound of Linus typing in all caps intensifies
- regularfry 6y agoThe 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 agoThat situation is one of the most costly mistakes, in every way i can think of, in software engineering period.
- magicalhippo 6y agoIMHO 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 agoI 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 agoNot 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 agoPeople 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 agoTrue. 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 agoIf 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 agoPow3, 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 agoAnd 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 agoit is IRRATIONAL code (maybe rational within context, but ...)
- akimball 6y agothrees_cursor fives_cursor sevens_cursor
- lifthrasiir 6y ago7 years ago: https://news.ycombinator.com/item?id=7296497 https://news.ycombinator.com/item?id=7296497
- asplake 6y agokeypusher on Feb 25, 2014: If only there was a system for fixing the code l yourself instead of having to post about it to a widely read tech site.
- GuB-42 6y agoAnother fun one is ONE = 256 (or another power of two). Intended for fixed point arithmetic where 1 actually means 1/256.
- ant6n 6y agoThis is nicer example, since the posted one should really have different variable names (e.g. pow3). On the other hand, if this declaration of ONE is global (not contained within some context like a type), then perhaps it should be something like ONE_F8.
- ferdek 6y agoSomething like ONE_FIX_UQ8? https://en.wikipedia.org/wiki/Q_%28number_format%29 https://en.wikipedia.org/wiki/Q_%28number_format%29
- Bost 6y agoIt's in the repository of a certain "torvalds". Better check twice next time what this guy's doing :)
- rkangel 6y agoI once spent two days debugging confusing behaviour in a serial command interface before discovering the line: #define CR_LF "\n"
- chasd00 6y agoman, that hurts. When you found the issue were you happy or were you just depressed? I've been there and sometimes finding the root of an issue is not a eureka moment but more just a long, deflating, sigh.
- rkangel 6y agoI was grumpy. Sometimes people are inexperienced and don't know how to make simple clean software and so end up with subtle bugs. I can understand that and dealing with it is part of life as an engineer. This was just laziness and there's no excuse for it. "Write software like the next person to work on it is a psychopath and they know where you live"
- augustk 6y agoDid you then rename the constant to NEWLINE to help the next programmer?
- deleted 6y ago[deleted]
- olarva 6y agoExplanatory comment: https://github.com/torvalds/linux/blob/d158fc7f36a25e19791d25a55da5623399a2644f/fs/ext4/resize.c#L653 https://github.com/torvalds/linux/blob/d158fc7f36a25e19791d2...
- brainfog 6y agoYikes, that looks error prone.
- dis-sys 6y agosurely we need another linter that can catch such issues
- known 6y agoThat's smart; Imagine user setting vm.swappiness = 100 in /etc/sysctl.conf
- karmakaze 6y agoThat was 2014, what does it say now?
- aaossa 6y agoNo changes: https://github.com/torvalds/linux/blob/master/fs/ext4/resize.c#L750-L752 https://github.com/torvalds/linux/blob/master/fs/ext4/resize...
- CWuestefeld 6y agoMany years ago in an age of klocs and flowcharts, at a large three-letter computer company, there were iron-clad coding rules that must be obeyed, no questions allowed. In general you could see their reasoning, but such bureaucratic reasoning doesn't pay off. One example of this was that all numeric values used in a program must be factored out as symbolic constants. The reason for doing this is obvious, but it failed to account for the fact that some numbers have intrinsic meaning that should allow their use directly. But to be compliant with the standard, our C code had this boilerplate up near the top: #define zero 0 #define one 1 This, of course, only served to make every page of code harder to read. And it didn't really even solve the problem it was meant to. We once found in some source code: #define thirteen 13 which of course did nothing to show us why that particular value was relevant.
- wycy 6y agoWe have something similar in my my organization's Fortran code. We have constants KI1, KI2, KI3, ... etc corresponding to (k)onstant integers 1, 2, 3, ... etc. Someone got the idea that hard-coded numbers were bad. All it does is make code harder to read to anyone unfamiliar with these conventions.
- hannasanarion 6y agoWhy is the reason for doing this obvious? I can't see any benefit. I get why you would prefer to name constants for their use, like `numIterations=3` or whatever, but renaming every integer seems senseless.
- yoz-y 6y agoIt's "obvious" if you take into account the corporate culture I guess. The rule of 'no magic numbers' is generally good, but then one has to remember that some numbers (like 0 and 1) are not magic.
- loa_in_ 6y ago0 and 1 can be magic if the context depends on them being arbitrarily 0 or 1. eg. // this byte is unused and always 0 #define UNUSED_CONSTANT_FIELD_VALUE 0 bytes = {HEADER, UNUSED_CONSTANT_FIELD_VALUE, [...]}
- tln 6y agoI wonder why a struct to hold the cursor state wasn't used.
- SavantIdiot 6y agoMagic numbers are a horrible pattern (and I don't mean elf/obj IDs). *ANY* time you use a non-obvious constant (like start of a loop that isn't 0), good practice to comment immediately at the source, or pull it into a well-named variable/macro. Don't agree? Spend 35 years programming and you'll see this pop up again and again and again and again... SO much time wasted figuring out magic #s.
- coding123 6y agoIt was done that way on purpose to cause hundreds of people a year to freak out and discuss it ad nauseum. But really a social experiment designed to show everyone is all talk and no follow through... That being that no one actually tried to change open source but just talk about it.
- ddtaylor 6y agothat's interesting, is there a source?
- amelius 6y agoFor small values of three.
- lgregg 6y agoLooks like it's still there: https://github.com/torvalds/linux/blob/master/fs/ext4/resize.c#L750 https://github.com/torvalds/linux/blob/master/fs/ext4/resize...
- sabhiram 6y agoIt's actually worse than that, given that all 3, 5 and 7 have 1 as their power-0, and the rest seem to encode powers of the former. Too much WTF and not enough comments.
- dyingkneepad 6y agoI don't know about the ext4 code specifically, but in hardware programming it is common to have stuff like "3 means 5". If for whatever reason a field of a register may only accept values that actually mean 1, 5 and 7 (to program whatever bits of the hardware, like clock divisors or whatnot), the hw register will probably only contain 2 bits, and be encoded in such a way where if you write 00b you mean 1d, if you write 01b you mean 5d and if you write 10d you mean 7. And 11b is reserved. This is a common thing, it's encoding to pack a sparse set of possible values into a few bits as you can. But then, you should probably do something such as "#define CLOCK_DIVISOR_3 0x1" instead of simply "int three = 1".
- trhway 6y agoto me it is a great message that if you have attitude that your predecessors were stupid morons and you don't understand what happens here nor willing to bother to dig enough deep to understand it, you shouldn't be doing changes here. Attempt to correct it is a kind of warning signal for a PR containing the correction.
- fargle 6y agoWhat wrong with: _3 = 1; It's shorter. I know, _ is probably reserved, so maybe: x3 = 1; // x is not multiply There. I fixed it for you.