avoid a variable shadowing warning #873
Conversation
Tests passed ✓, Code: 16678 B (+0.0%), Stack: 1432 B (+0.0%), Structs: 788 B (+0.0%)
|
|
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. |
|
actually, it's quite the opposite to "encourage programmers to increase a variable's scope more than necessary". |
|
Looking at the overall structure of the function, there are several instances of a variable called There are a few possible ways to go about this:
Overall, I'd prefer to see the second one implemented with P.S.: @geky There are several quite useful compiler flags not present in |
|
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 I think @BenBE's comment (#873 (comment)) does a good job of listing the possible options for resolving -Wshadow warnings:
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 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. |
|
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). |
|
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. |
No description provided.