avoid a variable shadowing warning

[geky-bot]

Tests passed ✓, Code: 16678 B (+0.0%), Stack: 1432 B (+0.0%), Structs: 788 B (+0.0%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16678 B (+0.0%) | 1432 B (+0.0%) | 788 B (+0.0%) | Lines | 2316/2496 lines (+0.0%) | | Readonly | 6126 B (+0.0%) | 448 B (+0.0%) | 788 B (+0.0%) | Branches | 1184/1506 branches (-0.0%) | | Threadsafe | 17506 B (+0.0%) | 1432 B (+0.0%) | 796 B (+0.0%) | | **Benchmarks** | | Multiversion | 16754 B (+0.0%) | 1432 B (+0.0%) | 792 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18362 B (+0.0%) | 1736 B (+0.0%) | 792 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17310 B (+0.0%) | 1424 B (+0.0%) | 788 B (+0.0%) | Erased | 1568888832 B (+0.0%) |

[geky]

Hi @yamt, thanks for creating a PR.

I'm not a fan of -Wshadow, I believe it does more harm than good. (Why would you encourage programmers to increase a variable's scope more than necessary?).

My current stance is to use GCC/Clang with -Wall -Wextra -pedantic and not use -Wshadow unless it is added to this set of default warnings in GCC or Clang.

I could merge this, but without -Wshadow in littlefs's CI it's likely a shadowed variable gets reintroduced in another patch. I believe the correct thing to do is disable the warning locally when compiling littlefs.

[PocketPi]

Hi @yamt, thanks for creating a PR.

I'm not a fan of -Wshadow, I believe it does more harm than good. (Why would you encourage programmers to increase a variable's scope more than necessary?).

My current stance is to use GCC/Clang with -Wall -Wextra -pedantic and not use -Wshadow unless it is added to this set of default warnings in GCC or Clang.

I could merge this, but without -Wshadow in littlefs's CI it's likely a shadowed variable gets reintroduced in another patch. I believe the correct thing to do is disable the warning locally when compiling littlefs.

I would love to have this PR merged as well.

And I do not agree that Wshadow "encourage programmers to increase a variable's scope more than necessary" imo it helps. If you want to keep the scope small then its better to change the variable name but still declare it in the small scope.

[yamt]

actually, it's quite the opposite to "encourage programmers to increase a variable's scope more than necessary". you can avoid the warning by reducing the scope of the shadowed variable so that it doesn't overlap with the shadowing one.

[BenBE]

Looking at the overall structure of the function, there are several instances of a variable called err. The one warning the compiler gives with -Wshadow is really just a symptom of this; and the patch only addresses part of the (actual) issue.

There are a few possible ways to go about this:

1. Move err to the outermost scope of the function and use it as a "global" temporary to store error state information (I don't like this, because this messes up scopes)
2. Narrow down the scope for each instance where err is used, by introducing a bare pair of braces around it. While this may look strange at first, this is most in spirit with what @geky said re reducing scopes
3. Rename each instance of err to have it's own variable name

Overall, I'd prefer to see the second one implemented with -Wshadow being added to the list of CI flags.

P.S.: @geky There are several quite useful compiler flags not present in -Wall -Wextra -pedantic that can provide an early notification about issues. Some of these include:

• -Wwrite-strings
• -Wcast-align
• -Wcast-qual
• -Wpointer-arith
• -Wstrict-aliasing
• -Wformat=2
• -Wuninitialized

[geky]

To give a concrete example, here is a relatively simple case where attempting to fix a -Wshadow warning would risk breaking code:

// close the floomeister
int floomeister_close(floomeister_t *floomeister) {
    int err = floomeister_freethepidgeons(floomeister);

    // if pidgeons cannot be freed, we need to wake the hibernating bear before we return
    if (err) {
        bear_t *bear = floomeister_getbear(floomeister);
        if (bear_ishibernating(bear)) {
            alarm_t *alarm = bear_getleastfavoritealarm(bear);

            int err = bear_wake(alarm);
            if (err) {
                return err;
            }
        }
    }

    // return the original error
    return err;
}

Situations like this can easily happen when copying complex chunks of code around. Keep in mind there may be more layers of code between the two declarations.

We should of course prefer simpler code paths when possible, and littlefs prefers to resolve err variables immediately, but this can't always be avoided and I think -Wshadow does more harm than good here.

I think @BenBE's comment (https://github.com/littlefs-project/littlefs/pull/873#issuecomment-1776772763) does a good job of listing the possible options for resolving -Wshadow warnings:

1. Move all err variables up to function scope - This is the unnecessary increase of scope I mentioned. It risks variables interacting in unintended ways, like in the above example.

2. Introducing a new scope for each err - This is probably the best solution in terms of bug-resistant code, and I'm a fan, but it comes with a readability cost. It also does not work with the above example.

3. Rename conflicting err instances - I believe this is the correct way to resolve the above example if you want -Wshadow. But these variables have the same purpose, so having a similar name helps readability. This leads to names like err1, err2, etc, and introduces the possiblity of accidentally using err5 when you mean err4. It also makes complicates copying chunks of code around.

---

It's interesting to note that Rust goes in the opposite direction, allowing shadowing even in the same scope (ref). If this were adopted in C, tightly-scoped err variables would become much more pleasant to write:

int do_many_things(void) {
    int err = do_thing_one();
    if (err) {
        return err;
    }

    int err = do_thing_two();
    if (err) {
        return err;
    }

    int err = do_thing_three();
    if (err) {
        return err;
    }

    return 0;
}

vs solution 2. with -Wshadow:

int do_many_things(void) {
    {
        int err = do_thing_one();
        if (err) {
            return err;
        }
    }
    {
        int err = do_thing_two();
        if (err) {
            return err;
        }
    }
    {
        int err = do_thing_three();
        if (err) {
            return err;
        }
    }

    return 0;
}

-Wshadow does protect against when you intended to mutate a variable in an outer scope, but from my experience this hasn't been that common? Maybe I need to use more global variables.

---

@BenBE, regarding more compiler flags: I'm not opposed to adopting warnings that are useful, but so far it seems like warnings not included in -Wextra have been excluded for good reason. Either because of questionable value, false positives, or, worst case, compiler bugs.

I think it would be more interesting to explore other static analysis tools, such as cppcheck. Limiting such tools (or more compiler flags) to CI would be a good compromise for producing safer releases without suffering from warning overload during development.

But IMO this is relatively low priority right now, since there is still a lot of code moving around to address littlefs's functional issues.

[BenBE]

From my experience, using quite a few extra flags in my code, the disadvantages are rare and I mostly see them as an extra layer of defense myself.

That said, together with a few more folks, I'll be looking through the littlefs source in the upcoming weeks*. What we have looked at so far looks promising (corse overview so far only), but following the issue tracker there are likely some places in the code we might need to follow up on. We'll provide feedback on our findings as soon as there's something noteworthy. @geky If you have any particular areas of interest that we should take a look at, feel free to ping me.

*usually only a few hours per week, idea based on this presentation (sorry, German only).

[geky]

More eyes are always welcome.

I'm not sure what areas would be best to look at though, maybe looking through the issue tracker for bugs where enough info is available would lead to something interesting? littlefs has become relatively stable recently, all things considered, short of changes that would require major API breakage.

Larger functionality work is currently going on in experimental branches, but those may be best described as TODO-soup at the moment.