Subzero: Add -Wshadow to the build.

src/IceInst.cpp

 //===- subzero/src/IceInst.cpp - High-level instruction implementation ----===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the Inst class, primarily the various
@@ skipping 52 matching lines @@
 } InstIcmpAttributes[] = {
 #define X(tag, str)                                                            \
   { str }                                                                      \
   ,
     ICEINSTICMP_TABLE
 #undef X
 };
 
 } // 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) {
   Number = isDeleted() ? NumberDeleted : Func->newInstNumber();
 }
 
 // Delete the instruction if its tentative Dead flag is still set
 // after liveness analysis.
 void Inst::deleteIfDead() {
   if (Dead)
@@ skipping 143 matching lines @@
             // has been sorted.
             LiveEnd->push_back(std::make_pair(VarNum, InstNumber));
           }
         }
       }
     }
   }
   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);
 }
 
 const char *InstArithmetic::getOpName(OpKind Op) {
   size_t OpIndex = static_cast<size_t>(Op);
   return OpIndex < InstArithmetic::_num
              ? InstArithmeticAttributes[OpIndex].DisplayString
              : "???";
 }
 
 bool InstArithmetic::isCommutative() const {
   return InstArithmeticAttributes[getOp()].IsCommutative;
 }
 
 InstAssign::InstAssign(Cfg *Func, Variable *Dest, Operand *Source)
     : InstHighLevel(Func, Inst::Assign, 1, Dest) {
   addSource(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 {
     addSource(Source);
   }
 }
 
 InstBr::InstBr(Cfg *Func, CfgNode *Target)
     : InstHighLevel(Func, Inst::Br, 0, nullptr), TargetFalse(Target),
       TargetTrue(nullptr) {}
@@ skipping 11 matching lines @@
   if (TargetFalse == OldNode) {
     TargetFalse = NewNode;
     return true;
   } else if (TargetTrue == OldNode) {
     TargetTrue = NewNode;
     return true;
   }
   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);
 }
 
 InstExtractElement::InstExtractElement(Cfg *Func, Variable *Dest,
                                        Operand *Source1, Operand *Source2)
     : InstHighLevel(Func, Inst::ExtractElement, 2, Dest) {
   addSource(Source1);
   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);
 }
 
 InstInsertElement::InstInsertElement(Cfg *Func, Variable *Dest,
                                      Operand *Source1, Operand *Source2,
                                      Operand *Source3)
     : InstHighLevel(Func, Inst::InsertElement, 3, Dest) {
   addSource(Source1);
   addSource(Source2);
@@ skipping 86 matching lines @@
   addSource(Addr);
   // The 3rd operand is a dummy placeholder for the RMW beacon.
   addSource(Data);
 }
 
 void InstStore::setRmwBeacon(Variable *Beacon) {
   Dest = llvm::dyn_cast<Variable>(getData());
   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);
   // Initialize in case buggy code doesn't set all entries
   for (SizeT I = 0; I < NumCases; ++I) {
     Values[I] = 0;
     Labels[I] = nullptr;
   }
 }
 
@@ skipping 23 matching lines @@
       Labels[I] = NewNode;
       return true;
     }
   }
   return false;
 }
 
 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) {}
 
 InstFakeDef::InstFakeDef(Cfg *Func, Variable *Dest, Variable *Src)
     : InstHighLevel(Func, Inst::FakeDef, Src ? 1 : 0, Dest) {
   assert(Dest);
   if (Src)
     addSource(Src);
 }
 
 InstFakeUse::InstFakeUse(Cfg *Func, Variable *Src)
     : InstHighLevel(Func, Inst::FakeUse, 1, nullptr) {
   assert(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)
     return IceType_void;
   return Dest->getType();
 }
 
 // ======================== Dump routines ======================== //
 
 void Inst::dumpDecorated(const Cfg *Func) const {
   if (!BuildDefs::dump())
     return;
   Ostream &Str = Func->getContext()->getStrDump();
   if (!Func->isVerbose(IceV_Deleted) && (isDeleted() || isRedundantAssign()))
     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 << "  ";
   if (isDeleted())
     Str << "  //";
   dump(Func);
   dumpExtras(Func);
   Str << "\n";
 }
 
@@ skipping 397 matching lines @@
     // preserve these.
     return true;
   }
   if (!Dest->hasReg() && !SrcVar->hasReg() &&
       Dest->getStackOffset() == SrcVar->getStackOffset())
     return true;
   return false;
 }
 
 } // end of namespace Ice