Hacker News Reader

Comment by hinoki | Hacker News Reader

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...

View on HN

I see a problem:

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???"

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.

People make typos and simple mistakes all the time, regardless of experience.

+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...

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.

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?

All three lines would be "insane" if the variables weren't mutated. As in a variable named "three" is insane if it solely contains either of the values 1 or 3.

When it contains other values it is still subpar variable naming.

If something looks insane, but probablu sane person made it and it works then that's usually good indicator that you are missing part of the picture and should look around. It still might be bad, but probably not insane.

When

  unsigned three = 1;

is accompanied by

  unsigned five = 5;
  unsigned seven = 7;

what would be the sane conclusion?

These are variables that will change anyway, so the initial values are probably not that relevant to their name?

In which case the names should be cnt0, cnt1, cnt2 or something along those lines.

That the code was edited more than once on its lifetime, and in one of those times (not the last one), by someone with pretty bad naming habits.

That there are some entities (maybe ordered, maybe related to numbers three, five and seven in some way) and there are some values associated with those entities, two of which (accidentally or not) need to be initially set to corresponding number, but the third one possibly not, but better look around in the comments or code to understand and make sure the code is correct.

Then maybe leave a comment right after `= 1;` why this variable shouldn't be initialized with 3 that would clear up confusion for you if it was there.

The reasonable conclusion is that you should read the surrounding context and try to understand what these variables are used for. The purpose of code review is to understand the code enough that you can spot real bugs; I try to avoid flagging surface-level stuff unless it's really important. IMO, this is a borderline case due to the very clear comment (which is a bit too far away).

And then, having located that comment, try to rename the variable to something more meaningful. Even gap1, gap2, and gap3 would be more informative.

We have a difference of opinion here. I lean away from bikeshedding in favor of finding more substantive issues. I'm also an old-timer and I'd call this a 3, on a scale from 1 (easily readable code) to 10 (a suitable IOCCC entry)

It's also true that unless I had a specific mandate to touch that code, I wouldn't. If nothing else it confuses the commit history, obscuring what I was actually in there to do.

But if it were my job to touch these lines of code, I would want to leave them better than I found them.

'Without digging deeper' isn't really what matters here. A better question would be 'after some very cursory further digging, are we worried?' The first thing I ask myself when I see something that looks as wrong as this does is "Is it possible for this piece of code to be completely broken, and for no one to have noticed yet?"

This is really a question about two processes - the one where people look at, read, edit and discuss the code, and the extent to which this is discoverable by them. And the one where people run the code and expect certain things to work and would notice if they didn't work, or didn't work correctly.

This fails both of those checks. This file has been edited 8 times in the last year, by 5 different people. It's not so long that several of them wouldn't have read through the whole thing. This bug, if it was a bug, is unlikely to have persisted for 6 years without being noticed. It's not obfuscated or deeply technical in any way.

Secondly, this is code which is fairly critical to millions of systems. Ext4s is in literally hundreds of millions of devices. It's used in data centers in contexts where people are likely to follow up on any unusual behavior by their filesystem. The code in question is likely to be very well exercised in tests. It's likely to be extremely well exercised by people doing research stress testing/comparison testing of filesystem code. It is undoubtedly very well exercised by real-world deployments.

The bar to assume that you know something the person who wrote the code didn't, as opposed to the other way around, should be very high here. Even taking all the benefit of the doubt (which you should always do, never assume that stuff 'probably works' without evidence) it's not hard to dismiss this.

The problem with this argument is that this particular snippet of code doesn't get run very often. It's the bit that works out where the superblock (and all its backups) are stored. Now, normally, only the one copy of the superblock is ever read - the backups are only read if the first copy of it is corrupted in some way. So almost all the time, the results of this code are not used. So if it were broken, then yes it could possibly go unnoticed for 6 years.

That's only a problem with one of the arguments, and is exactly why there are two approaches. Some errors are hard to detect by reading or working on the code, but easier to detect from behavior. Some are the other way around.

In this case, I do not agree that this could have gone unnoticed for a long time, even only looking at the behavior of the running system. When multiplied by the number of installations and how much filesystems are used, it is run a lot. It's also the kind of thing people investigate when it happens in certain settings - we lost a block and our whole filesystem was corrupted. And the kind of thing people test for.

> The first thing I ask myself when I see something that looks as wrong as this does is "Is it possible for this piece of code to be completely broken, and for no one to have noticed yet?"

Good reflex. In my first job out of school (big research lab), we would occasionally find odd bugs in the code base (as would be expected...bugs happen). The head of the project would often wonder, “how could the system ever have functioned with this bug? Someone must have deliberately tampered with the source to discredit us.”

The first few times I thought he was joking, but no: he seriously believed this and even tried to investigate.

This was years before source control, which would have answered the question.

Edit: fix iPad-caused typos

Three years ago, I was sent as a consultant to help a struggling small, local health insurance agency. They had a three man team, the lead had been there for fifteen or twenty years (built their system). He assured me on the first day that they didn't really have any bugs and that the system was pretty stable.

We were called in to help them because their management (and users) were aware they were underwater with the amount of bug fixes, live production data changes they made to keep things working, and impossibility of adding anything new to the system without tipping something else over.

It seems the attitude that "there's no chance we have any bugs" is pretty prevalent anywhere I've ever been, along with the usual mix of "how did this ever work" which I still always find pretty shocking to run across.

Lesson: dig deeper.

I agree. The relevant comment is far away.

It’s a local variable so it seems that there would be little impact in renaming it (and the other two). But if the local (kernel in this case) coding conventions discourage variable renaming, a brief comment referring to the later comment would be worthwhile.

Over commenting is a problem, but this would a good use case if renaming is not appropriate.

    unsigned three = 1;

To me, that is a 20 foot tall neon orange flashing Chesterton's fence. I think, "surely there is a very strong reason for this." Maybe not a good reason, but definitely a compelling one.

If I was code reviewing this, I would almost certainly insist on a different name to make the WTF go away. I’m not going to suggest one here despite having read the other code comment, as doing so would be bikeshedding — not my codebase and I have no skin in this — the only point I will insist on is that I cannot believe that “three” is a sensible choice of name for whatever it does. Not even what the comment implies it does.

Totally agree, it would definitely be better to rename it, but I read in another comment that the kernel is loathe to rename things. So then it makes you wonder, who allowed 'number_noun = N' in the first place?

"Three is the number of the counting, and the number thou shalt count to is three"

Why my kids ask me questions like this, "who invented the number three," I usually tell them it was someone vaguely European of the Enlightenment era with the same last name. "Ah, it was Ulysses Herman Three, inventor the natural numbers and famed bicyclist." Usually buys me enough time to look it up online.

Or at least require a comment explaining it, before someone needs to dig through the code.

Powers of three, five, and seven sounds a little bit like Totvald's very own little FizzBuzz game hidden in ext4's file-system source code.

> Totvald's very own little FizzBuzz game hidden in ext4's file-system source code

What's that?

Write a filesystem that saves all data blocks to disk, but:

- if the data block id is a multiple of 3, replace its contents with 0x03

- if the data block id is a multiple of 5, replace its contents with 0x05

- if the data block id is a multiple of both 3 and 5, replace its contents with 0x15

Take down the fence. It's in the filesystem code, what could go wrong?

> To me, that is a 20 foot tall neon orange flashing Chesterton's fence.

It's not a Chesterton's fence, though. The whole point of Chesterton's fence is that whoever put up the fence that you want to take down couldn't be bothered to add signage explaining why the fence was present, thus either it isn't important to keep the fence up long term, or they're a idiot, in which case there's no reason to belive they were correct in deciding that having a fence there was useful in the first place.

With this fence, if you walk a few meters to the side (read, "select 'ext4_list_backups' and hit ^F and ^G a few times); et viola, there's your signs.

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.

That situation is one of the most costly mistakes, in every way i can think of, in software engineering period.

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.

The sound of Linus typing in all caps intensifies

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

And the scope for "three" is quite small, a single short function. It's not like it's using a global variable or #define.

it is IRRATIONAL code (maybe rational within context, but ...)

threes_cursor fives_cursor sevens_cursor

Reply on news.ycombinator.com