Subzero: Add -Wshadow to the build.

Subzero: Add -Wshadow to the build.

BUG= none

[Jim Stichnoth, 2015-06-30 21:32:45]

stichnot@chromium.org changed reviewers:
+ jpp@chromium.org, jvoung@chromium.org, kschimpf@google.com, qining@google.com

[Jim Stichnoth, 2015-06-30 21:35:14]

This demonstrates a different coding style for avoiding shadowing, in which
arguments that shadow a member variable name are prefixed with "My".

Still looking for feedback / consensus about whether to move forward with this,
or just abandon it.

[jvoung (off chromium), 2015-07-01 18:44:51]

Well, I do like that -Wshadow catches things (bugs or unused vars)...

No strong opinion on which approach though =/  LLVM seems to have a bunch of
everything (_, vs My, for params vs members)...

E.g., the MC layer has a few instances of underscores in ctor argument names:

MCLEBFragment(const MCExpr &Value_, bool IsSigned_,
                MCSectionData *SD = nullptr)
   : MCFragment(FT_LEB, SD),
      Value(&Value_), IsSigned(IsSigned_) { Contents.push_back(0); }

But that was probably just the style that the early MC developer (Mr. Dunbar)
liked.

Immediately below that you have instead:

  MCDwarfLineAddrFragment(int64_t LineDelta, const MCExpr &AddrDelta,
                          MCSectionData *SD = nullptr)
      : MCFragment(FT_Dwarf, SD), LineDelta(LineDelta), AddrDelta(&AddrDelta) {
    Contents.push_back(0);
  }

introduced by someone other than Mr. Dunbar.

There are occasional other instances of _, but typically where the shadowing is
real:

void MachineRegionInfo::recalculate(MachineFunction &F,
                                    MachineDominatorTree *DT_,
                                    MachinePostDominatorTree *PDT_,
                                    MachineDominanceFrontier *DF_) {
  DT = DT_;
  PDT = PDT_;
  DF = DF_;
//...
}

There's a couple places where MyFoo is used, but that appears to be mostly for
local variables where shadowing could occur.

[Karl, 2015-07-07 17:00:40]

I'm not clear on what the rules for naming parameter/fields, and therefore I'm
having a hard time reviewing this.

https://codereview.chromium.org/1221643012/diff/20001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1221643012/diff/20001/Makefile.standalone#new...
Makefile.standalone:165: -Wshadow $(CXX_EXTRA)
Does '-Wshadow' need to be added to Makefile for toolchain_build_pnacl.py
builds? Is it not needed because a standalone build will point out the error?

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp#newcode240
src/IceInst.cpp:240: InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount,
uint32_t MyAlignInBytes,
Silly question. I don't understand why you choose to use My on MyAlignInBytes
but not Func and ByteCount (similar question on many other constructors in this
file).

https://codereview.chromium.org/1221643012/diff/20001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1221643012/diff/20001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:299: InstARM32(Cfg *Func, InstKindARM32 Kind, SizeT Maxsrcs,
Variable *Dest)
Again, I'm not sure what are the rules for naming parameters of constructors.
Why don't you use MyFunc, MyKind, MyMaxsrcs, and MyDest?

[John, 2015-07-07 17:07:49]

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp#newcode240
src/IceInst.cpp:240: InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount,
uint32_t MyAlignInBytes,
On 2015/07/07 17:00:40, Karl wrote:
> Silly question. I don't understand why you choose to use My on MyAlignInBytes
> but not Func and ByteCount (similar question on many other constructors in
this
> file).

AlignInBytes shadows a member variable (which LLVM's code convention does not
prevent.) Func and ByteCount, on the other hand, do not shadow any member
variables.

https://codereview.chromium.org/1221643012/diff/20001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1221643012/diff/20001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:299: InstARM32(Cfg *Func, InstKindARM32 Kind, SizeT Maxsrcs,
Variable *Dest)
On 2015/07/07 17:00:40, Karl wrote:
> Again, I'm not sure what are the rules for naming parameters of constructors.
> Why don't you use MyFunc, MyKind, MyMaxsrcs, and MyDest?

I believe the "My" prefix is only added when the parameter shadows the class
member.

[Karl, 2015-07-07 18:21:14]

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp#newcode240
src/IceInst.cpp:240: InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount,
uint32_t MyAlignInBytes,
On 2015/07/07 17:07:49, John wrote:
> On 2015/07/07 17:00:40, Karl wrote:
> > Silly question. I don't understand why you choose to use My on
MyAlignInBytes
> > but not Func and ByteCount (similar question on many other constructors in
> this
> > file).
> 
> AlignInBytes shadows a member variable (which LLVM's code convention does not
> prevent.) Func and ByteCount, on the other hand, do not shadow any member
> variables.

I disagree. Func is a protected inherited member variable. Doesn't it create the
same problem that shadow is intended to fix?

[John, 2015-07-07 18:28:32]

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp#newcode240
src/IceInst.cpp:240: InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount,
uint32_t MyAlignInBytes,
On 2015/07/07 18:21:13, Karl wrote:
> On 2015/07/07 17:07:49, John wrote:
> > On 2015/07/07 17:00:40, Karl wrote:
> > > Silly question. I don't understand why you choose to use My on
> MyAlignInBytes
> > > but not Func and ByteCount (similar question on many other constructors in
> > this
> > > file).
> > 
> > AlignInBytes shadows a member variable (which LLVM's code convention does
not
> > prevent.) Func and ByteCount, on the other hand, do not shadow any member
> > variables.
> 
> I disagree. Func is a protected inherited member variable. Doesn't it create
the
> same problem that shadow is intended to fix?

I don't see a

Ice::Inst::Func

neither a

Ice::InstHighLevel::Func

in IceInst.h

Am I missing something?

[John, 2015-07-07 18:39:09]

On 2015/07/07 18:28:32, John wrote:
> https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp
> File src/IceInst.cpp (right):
> 
>
https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp#newcode240
> src/IceInst.cpp:240: InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount,
> uint32_t MyAlignInBytes,
> On 2015/07/07 18:21:13, Karl wrote:
> > On 2015/07/07 17:07:49, John wrote:
> > > On 2015/07/07 17:00:40, Karl wrote:
> > > > Silly question. I don't understand why you choose to use My on
> > MyAlignInBytes
> > > > but not Func and ByteCount (similar question on many other constructors
in
> > > this
> > > > file).
> > > 
> > > AlignInBytes shadows a member variable (which LLVM's code convention does
> not
> > > prevent.) Func and ByteCount, on the other hand, do not shadow any member
> > > variables.
> > 
> > I disagree. Func is a protected inherited member variable. Doesn't it create
> the
> > same problem that shadow is intended to fix?
> 
> I don't see a
> 
> Ice::Inst::Func
> 
> neither a
> 
> Ice::InstHighLevel::Func
> 
> in IceInst.h
> 
> Am I missing something?

Your question got me thinking whether -Wshadow would work in case of an
inherited member. I added a Cfg *Func to Ice::Inst, and llvm reported that the
Func parameters shadow the inherited members. Thanks for bringing this up. :)

[Karl, 2015-07-07 19:09:34]

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp#newcode240
src/IceInst.cpp:240: InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount,
uint32_t MyAlignInBytes,
On 2015/07/07 18:28:32, John wrote:
> On 2015/07/07 18:21:13, Karl wrote:
> > On 2015/07/07 17:07:49, John wrote:
> > > On 2015/07/07 17:00:40, Karl wrote:
> > > > Silly question. I don't understand why you choose to use My on
> > MyAlignInBytes
> > > > but not Func and ByteCount (similar question on many other constructors
in
> > > this
> > > > file).
> > > 
> > > AlignInBytes shadows a member variable (which LLVM's code convention does
> not
> > > prevent.) Func and ByteCount, on the other hand, do not shadow any member
> > > variables.
> > 
> > I disagree. Func is a protected inherited member variable. Doesn't it create
> the
> > same problem that shadow is intended to fix?
> 
> I don't see a
> 
> Ice::Inst::Func
> 
> neither a
> 
> Ice::InstHighLevel::Func
> 
> in IceInst.h
> 
> Am I missing something?

Ok. I'm wrong about Func. However Dest is defined in class Inst. This points out
the problem that I'm am having with the current convention. To determine when to
avoid this problem that shadow is trying to fix, you must look deep into the
code (via inheritance) to see when the My prefix should be used. In addition, it
leaves the code prone to a code change that may change where inherited
constructors are used.

What I would like is some rule that is simply to apply. For example, I would
assume that any parameter set to a direct field, or passed to a derived class
should have the My prefix.

[John, 2015-07-07 19:29:44]

On 2015/07/07 19:09:34, Karl wrote:
> https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp
> File src/IceInst.cpp (right):
> 
>
https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp#newcode240
> src/IceInst.cpp:240: InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount,
> uint32_t MyAlignInBytes,
> On 2015/07/07 18:28:32, John wrote:
> > On 2015/07/07 18:21:13, Karl wrote:
> > > On 2015/07/07 17:07:49, John wrote:
> > > > On 2015/07/07 17:00:40, Karl wrote:
> > > > > Silly question. I don't understand why you choose to use My on
> > > MyAlignInBytes
> > > > > but not Func and ByteCount (similar question on many other
constructors
> in
> > > > this
> > > > > file).
> > > > 
> > > > AlignInBytes shadows a member variable (which LLVM's code convention
does
> > not
> > > > prevent.) Func and ByteCount, on the other hand, do not shadow any
member
> > > > variables.
> > > 
> > > I disagree. Func is a protected inherited member variable. Doesn't it
create
> > the
> > > same problem that shadow is intended to fix?
> > 
> > I don't see a
> > 
> > Ice::Inst::Func
> > 
> > neither a
> > 
> > Ice::InstHighLevel::Func
> > 
> > in IceInst.h
> > 
> > Am I missing something?
> 
> Ok. I'm wrong about Func. However Dest is defined in class Inst. This points
out
> the problem that I'm am having with the current convention. To determine when
to
> avoid this problem that shadow is trying to fix, you must look deep into the
> code (via inheritance) to see when the My prefix should be used. In addition,
it
> leaves the code prone to a code change that may change where inherited
> constructors are used.
> 
> What I would like is some rule that is simply to apply. For example, I would
> assume that any parameter set to a direct field, or passed to a derived class
> should have the My prefix.

It turns out that this patch does not compile (not on my machine at least.) You
don't have to inspect the class hierarchy: the compiler will warn you about
shadow when it happens.

I believe the "convention" is to rename a parameter whenever you try to compile
your code and get a compile time error. "My" is both short and descriptive (and
it does not violate LLVM rules afaik.)

Moreover, I am not trying to "impose" anything: I am just stepping in for Jim
while he's out. I am sure he will have more to say. :)

[Jim Stichnoth, 2015-07-13 19:33:37]

Rebased the patch.

To repeat some comments on the CL, the idea is to catch errors and sloppiness by
adding -Wshadow to the standalone build, and then fix warnings/errors by
renaming variables (or whatever) as needed.  The vast majority of new warnings
are from ctor args, and these are fixed using the "My" prefix.

I'm also looking for consensus on whether -Wshadow is even a good idea, i.e.
worth the extra hassle and ugliness.

https://codereview.chromium.org/1221643012/diff/20001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1221643012/diff/20001/Makefile.standalone#new...
Makefile.standalone:165: -Wshadow $(CXX_EXTRA)
On 2015/07/07 17:00:40, Karl wrote:
> Does '-Wshadow' need to be added to Makefile for toolchain_build_pnacl.py
> builds? Is it not needed because a standalone build will point out the error?

I meant this only for the standalone build.  I don't think it's such a good idea
for the standard build, especially when e.g. Windows uses g++ where the
warnings/errors may be different.

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1221643012/diff/20001/src/IceInst.cpp#newcode240
src/IceInst.cpp:240: InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount,
uint32_t MyAlignInBytes,
On 2015/07/07 19:09:34, Karl wrote:
> On 2015/07/07 18:28:32, John wrote:
> > On 2015/07/07 18:21:13, Karl wrote:
> > > On 2015/07/07 17:07:49, John wrote:
> > > > On 2015/07/07 17:00:40, Karl wrote:
> > > > > Silly question. I don't understand why you choose to use My on
> > > MyAlignInBytes
> > > > > but not Func and ByteCount (similar question on many other
constructors
> in
> > > > this
> > > > > file).
> > > > 
> > > > AlignInBytes shadows a member variable (which LLVM's code convention
does
> > not
> > > > prevent.) Func and ByteCount, on the other hand, do not shadow any
member
> > > > variables.
> > > 
> > > I disagree. Func is a protected inherited member variable. Doesn't it
create
> > the
> > > same problem that shadow is intended to fix?
> > 
> > I don't see a
> > 
> > Ice::Inst::Func
> > 
> > neither a
> > 
> > Ice::InstHighLevel::Func
> > 
> > in IceInst.h
> > 
> > Am I missing something?
> 
> Ok. I'm wrong about Func. However Dest is defined in class Inst. This points
out
> the problem that I'm am having with the current convention. To determine when
to
> avoid this problem that shadow is trying to fix, you must look deep into the
> code (via inheritance) to see when the My prefix should be used. In addition,
it
> leaves the code prone to a code change that may change where inherited
> constructors are used.
> 
> What I would like is some rule that is simply to apply. For example, I would
> assume that any parameter set to a direct field, or passed to a derived class
> should have the My prefix.

I think the rules might read something like this:

1. Turn on -Wshadow in conjunction with -Werror.

2. Resolve shadowing errors in ctors by adding the "My" prefix to ctor arguments
as necessary.  This only needs to be done for arguments that cause a shadowing
warning/error.

3. Resolve other shadowing errors by using other renaming rules of your choice,
or fix / clean up accidents that -Wshadow reveals.

https://codereview.chromium.org/1221643012/diff/20001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1221643012/diff/20001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:299: InstARM32(Cfg *Func, InstKindARM32 Kind, SizeT Maxsrcs,
Variable *Dest)
On 2015/07/07 17:07:49, John wrote:
> On 2015/07/07 17:00:40, Karl wrote:
> > Again, I'm not sure what are the rules for naming parameters of
constructors.
> > Why don't you use MyFunc, MyKind, MyMaxsrcs, and MyDest?
> 
> I believe the "My" prefix is only added when the parameter shadows the class
> member.

Right.  The idea is that you generally name arguments as you like, but rename
them (perhaps using the My prefix) to fix warnings.

[jvoung (off chromium), 2015-07-13 22:40:43]

https://codereview.chromium.org/1221643012/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1221643012/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1270: T_Hi = makeReg(IceType_i32);
oops, T_Hi and T_Lo should already be temp regs from line 1174/1175

https://codereview.chromium.org/1221643012/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1309: T_Lo = makeReg(IceType_i32);
same

[Jim Stichnoth, 2015-07-13 23:01:04]

https://codereview.chromium.org/1221643012/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1221643012/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1270: T_Hi = makeReg(IceType_i32);
On 2015/07/13 22:40:43, jvoung wrote:
> oops, T_Hi and T_Lo should already be temp regs from line 1174/1175

Good point.  I kept the assignment because 1174/1175 use DestLo->getType() while
this assignment uses IceType_i32.  But now I realize that those will be the same
since this is i64 lowering.

https://codereview.chromium.org/1221643012/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1309: T_Lo = makeReg(IceType_i32);
On 2015/07/13 22:40:43, jvoung wrote:
> same

Done.

[jvoung (off chromium), 2015-07-14 01:25:19]

I would be okay with it, but I haven't "experienced" it / had to face any of the
warnings and rename stuff. It's definitely a tradeoff...

It is too bad we would have to add the "My" in a bunch of places, and not just
constructors. On the other hand, a bunch of the non-ctor edits are to remove
variables so they aren't *adding* My.

As for what google/chromium does (not that it helps because we are LLVM
style)... its style is underscore the fields. That would be a straightforward
rule of "just do it all the time", but then a ton more code will have to be
changed and a ton more code will then have extra styling of "_".

As for alternative warning mechanisms, -Wself-assign wouldn't have caught the
earlier bug, and clang's scan-build static analysis doesn't catch it either =(

Either way, some of these edits could surely go in (removing dead code!), but
this did help to see what types of things Wshadow would catch.

https://codereview.chromium.org/1221643012/diff/60001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/1221643012/diff/60001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:811: void Converter::nameUnnamedGlobalVariables(Module
*MyMod) {
This could probably just use Mod instead of taking a parameter (should be the
same Mod?)?

Similar below?

https://codereview.chromium.org/1221643012/diff/60001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1221643012/diff/60001/src/IceInst.h#newcode398
src/IceInst.h:398: bool MyHasTailCall, bool HasSideEff, InstKind Kind)
I suppose HasSideEff can be MyHasSideEffects (it was working around the
shadowing from the base class).

[Jim Stichnoth, 2015-07-24 14:13:41]

Given the lack of consensus on this (and tbh, I'm not completely bought into
this approach either), I think I'll just abandon this and close the CL.