Thread: Re: Remove shadowed declaration warnings
On Thu, 12 Sept 2024 at 12:33, Peter Smith <smithpb2250@gmail.com> wrote: > I normally build the code with warnings enabled (specifically, > -Wshadow) which exposes many "shadowed" declarations. > > It would be better to reduce warnings wherever it's easy to do so, > because if we always see/ignore lots of warnings then sooner or later > something important may escape attention. In any case, just removing > these warnings sometimes makes the code more readable by removing any > ambiguity. 0fe954c28 did add -Wshadow=compatible-local to the standard set of complication flags. I felt it was diminishing returns after that, but -Wshadow=local would be the next step before going full -Wshadow. There was justification for -Wshadow=compatible-local because there has been > 1 bug (see af7d270dd) fixed that would have been found if we'd had that sooner. Have we ever had any bugs that would have been highlighted by -Wshadow but not -Wshadow=compatible-local? I'd be curious to know if you do go through this process of weeding these out if you do find any bugs as a result. I also wonder if we could ever get this to a stable point. I didn't take the time to understand what 388e80132 did. Is that going to protect us from getting warnings where fixing them is beyond our control for full -Wshadow? David
David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 12 Sept 2024 at 12:33, Peter Smith <smithpb2250@gmail.com> wrote:
>> I normally build the code with warnings enabled (specifically,
>> -Wshadow) which exposes many "shadowed" declarations.
> 0fe954c28 did add -Wshadow=compatible-local to the standard set of
> complication flags. I felt it was diminishing returns after that, but
> -Wshadow=local would be the next step before going full -Wshadow.
I think that insisting that local declarations not shadow globals
is an anti-pattern, and I'll vote against any proposal to make
that a standard warning. Impoverished as C is, it does have block
structure; why would we want to throw that away by (in effect)
demanding a single flat namespace for the entire program?
I do grant that sometimes shadowing of locals can cause bugs. I don't
recall right now why we opted for -Wshadow=compatible-local over
-Wshadow=local, but we could certainly take another look at that.
regards, tom lane
On Thu, 12 Sept 2024 at 14:03, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I do grant that sometimes shadowing of locals can cause bugs. I don't > recall right now why we opted for -Wshadow=compatible-local over > -Wshadow=local, but we could certainly take another look at that. I don't recall if it was discussed, but certainly, it was an easier goal to achieve. Looks like there are currently 47 warnings with -Wshadow=local. I'm not quite sure what the definition of "compatible" is for this flag, but looking at one warning in pgbench.c:4548, I see an int shadowing a bool. So maybe -Wshadow=local is worthwhile. David
On 12.09.24 04:25, David Rowley wrote: > On Thu, 12 Sept 2024 at 14:03, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I do grant that sometimes shadowing of locals can cause bugs. I don't >> recall right now why we opted for -Wshadow=compatible-local over >> -Wshadow=local, but we could certainly take another look at that. > > I don't recall if it was discussed, but certainly, it was an easier > goal to achieve. > > Looks like there are currently 47 warnings with -Wshadow=local. I'm > not quite sure what the definition of "compatible" is for this flag, > but looking at one warning in pgbench.c:4548, I see an int shadowing a > bool. So maybe -Wshadow=local is worthwhile. Another thing to keep in mind with these different shadow warning variants is that we should try to keep them consistent across compilers, at least the common ones. The current -Wshadow=compatible-local is only available in gcc, not in clang, so it's currently impossible to rely on clang to write warning-free code. Of course we have other warning flags that we use that don't exist in all compilers, but in my experience these are either for very esoteric cases or something that is very obviously wrong and a developer would normally see immediately. For example, there is no warning flag in clang that mirrors the switch "fallthrough" labeling that we have set up with gcc. But this is not as much of a problem in practice because the wrong code would usually misbehave in an obvious way and the issue can be found by looking at the code with two lines of context. With the shadow warning, the issues are much harder to find visually, and in most cases they are not actually a problem. The shadow warning levels in gcc and clang appear to be very differently structured, so I'm not sure how we can improve interoperability here.