This one could be helped by always using this pattern whenever you write a function that returns a value, in any language, along with no early returns:

int func(...) {
    int result = -1;
    ...
    return result;
}

I always start with writing my result default value, ideally indicating failure, and the return line. Then I implement the rest. We often don't have the luxury of choosing the language we work with that has the features we like, but consistently enforced code style can help with a lot of problems. Anyone can make mistakes like the one in this bug regardless of experience so every little bit helps.

[–] elmicha@feddit.org 9 points 5 days ago (1 children)

But what would that help in this situation? You could as easily write result = asize; and the psize would again be unused.

[–] avidamoeba@lemmy.ca 8 points 5 days ago* (last edited 5 days ago)

You could absolutely do that and be fucked too. However the point of the pattern I suggested isn't to replace the return with an assignment. That is, the point isn't to do the exact same implementation and then do result = something before returning it. Instead it's to use the initialized result var to directly store your result throughout the function, at every place where you manipulate it. So in this case my suggestion is to not have psize at all. Instead start with int result = -1; and return result; and do all the things you do to psize except on result. Then there's a higher chance you will return the right value. Not a guarantee. I'm not at all implying that "if they only did this one thing, they wouldn't have fucked up like this, so stupid" I'm merely suggesting a style that can decrease the probability of this type of error, in my experience. I'm teaching my team to write in defensive ways so they can feel some confidence in what they wrote, even if they slept 2 hours the night before, and also understand it after another bad night. Cause that ends up happening, life happens and like OpenZFS we also can't afford serious bugs in what we do.

[–] DapperPenguin@programming.dev 13 points 5 days ago* (last edited 5 days ago) (1 children)

I appreciate rust as much as the next dev. But you can define your own types in C just as well? And with the proper warnings and errors -Wall -Werror in place, any warning is an error, and implicit conversions should probably be a warning right?

ETA: Just tried with the following C code and could not get it to fail with gcc.

typedef int t_0;
typedef long t_1;

t_0 test() {
  t_1 foo = 1;
  return foo;
}

Tried with gcc -Wall -Wextra -Wpedantic -Werror and it compiled just fine.

[–] brisk@aussie.zone 4 points 4 days ago

typedef in C just make an alias to the same type. structs have nominal typing though:

// this typedef is optional to avoid having to refer to the struct tag when referencing the types
typedef struct {int} t_0;
typedef struct {long} t_1;

t_0 test() {
  t_1 foo = {1};
  return foo; // error
}

[–] kcuf2@lemmynsfw.com 6 points 5 days ago (1 children)

No unit test?

[–] FizzyOrange@programming.dev 10 points 5 days ago (1 children)

The article said it pretty well:

if your answer to any perceived failing in a person is “just try harder”, you are either woefully inexperienced or a just a dick

That applies to writing impossibly comprehensive unit tests too.

Though really for a filesystem they should really do silicon-style verification (which we're calling Deterministic System Testing now).

[–] kcuf2@lemmynsfw.com 0 points 5 days ago

This is a pretty straightforward utility function that wouldn't be that hard to test. It's normal to have standards for code coverage as part of the review process, I don't code in C but I'd be surprised if setting any of that up is actually that burdensome.

I disagree with your reference of that quote being applicable here though. In fact, adding unit tests is the exact opposite: that quote is saying 'if your answer is "make less mistakes" ....' Unit tests in general are an acceptance of the fact that we will fail to be perfect and we need to mitigate that with extra checks. The article already said it had two human reviewers, so they're not opposed to extra process to help ensure quality, unit tests are just another (actually very cheap) extra step.

[–] teppa@piefed.ca 6 points 5 days ago

Cognitive rigidity is a hell of a thing.