Subzero: Add -Wshadow to the build.

src/IceInst.cpp

Index: src/IceInst.cpp
diff --git a/src/IceInst.cpp b/src/IceInst.cpp
index 317232a12256764acd78ca4ad82448077020dded..c2e9877d4e842c6fd8740e8fa5b3557c912be873 100644
--- a/src/IceInst.cpp
+++ b/src/IceInst.cpp
@@ -70,9 +70,10 @@ const struct InstIcmpAttributes_ {
 
 } // end of anonymous namespace
 
-Inst::Inst(Cfg *Func, InstKind Kind, SizeT MaxSrcs, Variable *Dest)
-    : Kind(Kind), Number(Func->newInstNumber()), Dest(Dest), MaxSrcs(MaxSrcs),
-      Srcs(Func->allocateArrayOf<Operand *>(MaxSrcs)), LiveRangesEnded(0) {}
+Inst::Inst(Cfg *Func, InstKind MyKind, SizeT MyMaxSrcs, Variable *MyDest)
+    : Kind(MyKind), Number(Func->newInstNumber()), Dest(MyDest),
+      MaxSrcs(MyMaxSrcs), Srcs(Func->allocateArrayOf<Operand *>(MyMaxSrcs)),
+      LiveRangesEnded(0) {}
 
 // Assign the instruction a new number.
 void Inst::renumber(Cfg *Func) {
@@ -236,17 +237,17 @@ bool Inst::liveness(InstNumberT InstNumber, LivenessBV &Live,
   return true;
 }
 
-InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount, uint32_t AlignInBytes,
+InstAlloca::InstAlloca(Cfg *Func, Operand *ByteCount, uint32_t MyAlignInBytes,

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

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).

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

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.

[Karl, 2015/07/07 18:21:13]

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]

I don't see a

Ice::Inst::Func

neither a

Ice::InstHighLevel::Func

in IceInst.h

Am I missing something?

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

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.

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

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.

                        Variable *Dest)
-    : InstHighLevel(Func, Inst::Alloca, 1, Dest), AlignInBytes(AlignInBytes) {
+    : InstHighLevel(Func, Inst::Alloca, 1, Dest), AlignInBytes(MyAlignInBytes) {
   // Verify AlignInBytes is 0 or a power of 2.
   assert(AlignInBytes == 0 || llvm::isPowerOf2_32(AlignInBytes));
   addSource(ByteCount);
 }
 
-InstArithmetic::InstArithmetic(Cfg *Func, OpKind Op, Variable *Dest,
+InstArithmetic::InstArithmetic(Cfg *Func, OpKind MyOp, Variable *Dest,
                                Operand *Source1, Operand *Source2)
-    : InstHighLevel(Func, Inst::Arithmetic, 2, Dest), Op(Op) {
+    : InstHighLevel(Func, Inst::Arithmetic, 2, Dest), Op(MyOp) {
   addSource(Source1);
   addSource(Source2);
 }
@@ -270,10 +271,10 @@ InstAssign::InstAssign(Cfg *Func, Variable *Dest, Operand *Source)
 // If TargetTrue==TargetFalse, we turn it into an unconditional
 // branch.  This ensures that, along with the 'switch' instruction
 // semantics, there is at most one edge from one node to another.
-InstBr::InstBr(Cfg *Func, Operand *Source, CfgNode *TargetTrue_,
-               CfgNode *TargetFalse_)
-    : InstHighLevel(Func, Inst::Br, 1, nullptr), TargetFalse(TargetFalse_),
-      TargetTrue(TargetTrue_) {
+InstBr::InstBr(Cfg *Func, Operand *Source, CfgNode *MyTargetTrue,
+               CfgNode *MyTargetFalse)
+    : InstHighLevel(Func, Inst::Br, 1, nullptr), TargetFalse(MyTargetFalse),
+      TargetTrue(MyTargetTrue) {
   if (TargetTrue == TargetFalse) {
     TargetTrue = nullptr; // turn into unconditional version
   } else {
@@ -305,8 +306,9 @@ bool InstBr::repointEdge(CfgNode *OldNode, CfgNode *NewNode) {
   return false;
 }
 
-InstCast::InstCast(Cfg *Func, OpKind CastKind, Variable *Dest, Operand *Source)
-    : InstHighLevel(Func, Inst::Cast, 1, Dest), CastKind(CastKind) {
+InstCast::InstCast(Cfg *Func, OpKind MyCastKind, Variable *Dest,
+                   Operand *Source)
+    : InstHighLevel(Func, Inst::Cast, 1, Dest), CastKind(MyCastKind) {
   addSource(Source);
 }
 
@@ -317,16 +319,16 @@ InstExtractElement::InstExtractElement(Cfg *Func, Variable *Dest,
   addSource(Source2);
 }
 
-InstFcmp::InstFcmp(Cfg *Func, FCond Condition, Variable *Dest, Operand *Source1,
-                   Operand *Source2)
-    : InstHighLevel(Func, Inst::Fcmp, 2, Dest), Condition(Condition) {
+InstFcmp::InstFcmp(Cfg *Func, FCond MyCondition, Variable *Dest,
+                   Operand *Source1, Operand *Source2)
+    : InstHighLevel(Func, Inst::Fcmp, 2, Dest), Condition(MyCondition) {
   addSource(Source1);
   addSource(Source2);
 }
 
-InstIcmp::InstIcmp(Cfg *Func, ICond Condition, Variable *Dest, Operand *Source1,
-                   Operand *Source2)
-    : InstHighLevel(Func, Inst::Icmp, 2, Dest), Condition(Condition) {
+InstIcmp::InstIcmp(Cfg *Func, ICond MyCondition, Variable *Dest,
+                   Operand *Source1, Operand *Source2)
+    : InstHighLevel(Func, Inst::Icmp, 2, Dest), Condition(MyCondition) {
   addSource(Source1);
   addSource(Source2);
 }
@@ -433,10 +435,10 @@ void InstStore::setRmwBeacon(Variable *Beacon) {
   Srcs[2] = Beacon;
 }
 
-InstSwitch::InstSwitch(Cfg *Func, SizeT NumCases, Operand *Source,
-                       CfgNode *LabelDefault)
-    : InstHighLevel(Func, Inst::Switch, 1, nullptr), LabelDefault(LabelDefault),
-      NumCases(NumCases) {
+InstSwitch::InstSwitch(Cfg *Func, SizeT MyNumCases, Operand *Source,
+                       CfgNode *MyLabelDefault)
+    : InstHighLevel(Func, Inst::Switch, 1, nullptr),
+      LabelDefault(MyLabelDefault), NumCases(MyNumCases) {
   addSource(Source);
   Values = Func->allocateArrayOf<uint64_t>(NumCases);
   Labels = Func->allocateArrayOf<CfgNode *>(NumCases);
@@ -480,9 +482,9 @@ bool InstSwitch::repointEdge(CfgNode *OldNode, CfgNode *NewNode) {
 InstUnreachable::InstUnreachable(Cfg *Func)
     : InstHighLevel(Func, Inst::Unreachable, 0, nullptr) {}
 
-InstBundleLock::InstBundleLock(Cfg *Func, InstBundleLock::Option BundleOption)
+InstBundleLock::InstBundleLock(Cfg *Func, InstBundleLock::Option MyBundleOption)
     : InstHighLevel(Func, Inst::BundleLock, 0, nullptr),
-      BundleOption(BundleOption) {}
+      BundleOption(MyBundleOption) {}
 
 InstBundleUnlock::InstBundleUnlock(Cfg *Func)
     : InstHighLevel(Func, Inst::BundleUnlock, 0, nullptr) {}
@@ -500,8 +502,8 @@ InstFakeUse::InstFakeUse(Cfg *Func, Variable *Src)
   addSource(Src);
 }
 
-InstFakeKill::InstFakeKill(Cfg *Func, const Inst *Linked)
-    : InstHighLevel(Func, Inst::FakeKill, 0, nullptr), Linked(Linked) {}
+InstFakeKill::InstFakeKill(Cfg *Func, const Inst *MyLinked)
+    : InstHighLevel(Func, Inst::FakeKill, 0, nullptr), Linked(MyLinked) {}
 
 Type InstCall::getReturnType() const {
   if (Dest == nullptr)
@@ -519,11 +521,10 @@ void Inst::dumpDecorated(const Cfg *Func) const {
     return;
   if (Func->isVerbose(IceV_InstNumbers)) {
     char buf[30];
-    InstNumberT Number = getNumber();
-    if (Number == NumberDeleted)
+    if (getNumber() == NumberDeleted)
       snprintf(buf, llvm::array_lengthof(buf), "[XXX]");
     else
-      snprintf(buf, llvm::array_lengthof(buf), "[%3d]", Number);
+      snprintf(buf, llvm::array_lengthof(buf), "[%3d]", getNumber());
     Str << buf;
   }
   Str << "  ";