[llvm] r234963 - Change range-based for-loops to be -Wrange-loop-analysis clean.
David Blaikie
dblaikie at gmail.com
Wed Apr 15 11:48:53 PDT 2015
On Wed, Apr 15, 2015 at 11:39 AM, Richard Trieu <rtrieu at google.com> wrote:
>
>
> On Tue, Apr 14, 2015 at 9:36 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Tue, Apr 14, 2015 at 9:18 PM, Richard Trieu <rtrieu at google.com> wrote:
>> >
>> >
>> > On Tue, Apr 14, 2015 at 8:28 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> >>
>> >> On Tue, Apr 14, 2015 at 8:21 PM, Richard Trieu <rtrieu at google.com>
>> wrote:
>> >> > On Tue, Apr 14, 2015 at 6:33 PM, David Blaikie <dblaikie at gmail.com>
>> >> > wrote:
>> >> >>
>> >> >> On Tue, Apr 14, 2015 at 6:21 PM, Richard Trieu <rtrieu at google.com>
>> >> >> wrote:
>> >> >> > Author: rtrieu
>> >> >> > Date: Tue Apr 14 20:21:15 2015
>> >> >> > New Revision: 234963
>> >> >> >
>> >> >> > URL: http://llvm.org/viewvc/llvm-project?rev=234963&view=rev
>> >> >> > Log:
>> >> >> > Change range-based for-loops to be -Wrange-loop-analysis clean.
>> >> >> > No functionality change.
>> >> >> >
>> >> >> > Modified:
>> >> >> > llvm/trunk/lib/Analysis/RegionPass.cpp
>> >> >> > llvm/trunk/lib/CodeGen/WinEHPrepare.cpp
>> >> >> > llvm/trunk/lib/MC/MCDwarf.cpp
>> >> >> > llvm/trunk/lib/Object/COFFObjectFile.cpp
>> >> >> > llvm/trunk/lib/Target/AArch64/AArch64CollectLOH.cpp
>> >> >> > llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp
>> >> >> > llvm/trunk/tools/llvm-cxxdump/llvm-cxxdump.cpp
>> >> >> > llvm/trunk/utils/TableGen/CodeGenRegisters.cpp
>> >> >> >
>> >> >> > Modified: llvm/trunk/lib/Analysis/RegionPass.cpp
>> >> >> > URL:
>> >> >> >
>> >> >> >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/RegionPass.cpp?rev=234963&r1=234962&r2=234963&view=diff
>> >> >> >
>> >> >> >
>> >> >> >
>> ==============================================================================
>> >> >> > --- llvm/trunk/lib/Analysis/RegionPass.cpp (original)
>> >> >> > +++ llvm/trunk/lib/Analysis/RegionPass.cpp Tue Apr 14 20:21:15
>> 2015
>> >> >> > @@ -199,7 +199,7 @@ public:
>> >> >> >
>> >> >> > bool runOnRegion(Region *R, RGPassManager &RGM) override {
>> >> >> > Out << Banner;
>> >> >> > - for (const auto &BB : R->blocks()) {
>> >> >> > + for (const auto *BB : R->blocks()) {
>> >> >> > if (BB)
>> >> >> > BB->print(Out);
>> >> >> > else
>> >> >> >
>> >> >> > Modified: llvm/trunk/lib/CodeGen/WinEHPrepare.cpp
>> >> >> > URL:
>> >> >> >
>> >> >> >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/WinEHPrepare.cpp?rev=234963&r1=234962&r2=234963&view=diff
>> >> >> >
>> >> >> >
>> >> >> >
>> ==============================================================================
>> >> >> > --- llvm/trunk/lib/CodeGen/WinEHPrepare.cpp (original)
>> >> >> > +++ llvm/trunk/lib/CodeGen/WinEHPrepare.cpp Tue Apr 14 20:21:15
>> 2015
>> >> >> > @@ -909,7 +909,7 @@ bool WinEHPrepare::outlineHandler(Action
>> >> >> > // save the association of the blocks in LPadTargetBlocks.
>> The
>> >> >> > // return instructions which are created from these branches
>> >> >> > will
>> >> >> > be
>> >> >> > // replaced after all landing pads have been outlined.
>> >> >> > - for (const auto &MapEntry : VMap) {
>> >> >> > + for (const auto MapEntry : VMap) {
>> >> >>
>> >> >> What's the deal with this change? Is the type small & trivially
>> >> >> copyable & we're suggesting using values instead of references?
>> >> >>
>> >> > VMap does not return a reference, so remove the reference type to
>> >> > indicate a
>> >> > copy is always made.
>> >>
>> >> Seems ValueMap uses a proxy (much like std::vector<bool>/bitset/etc).
>> >>
>> >> How much value do you think there is in warning when a reference is
>> >> bound to a value return from operator*? I appreciate that it's useful
>> >> when there's an intentional type difference (though that's tricky -
>> >> are we warning there now? I forget) but if the types match it's sort
>> >> of a benign reference, right?
>> >>
>> > I think it's mostly benign, as long as the container isn't changed and
>> the
>> > code expects the new value,
>>
>> Actually that's the trick - because these things are proxied, the code
>> looks like it makes a copy of the container elements, but it actually
>> doesn't:
>>
>> for (auto x : my_vec_bool_or_bitset)
>> x = 0;
>>
>> Does that code modify the container? Surprisingly, it does... - so I'm
>> not sure it's helpful to remove the ref qualifier. While, yes,
>> including the ref qualifier is a lie, I suspect it might be the lesser
>> evil here... maybe. (granted you can use "const auto &x" and then
>> write "x = 0;" and that will compile, and similarly be confusing to
>> the author... so I'm not saying I have an iron-clad argument here)
>>
>> It looks like if there is "const auto &x" or "const auto x" for
> vector<bool>, then "x = 0;" won't compile. I wouldn't mind supporting
> "const auto &x" for this case, but is there a way to detect these proxy
> types?
>
I don't think there's anything in particular we could do (maybe look for
op= overloads?) to catch these cases, beyond just any by-value-returning
iterator.
My understanding was that the purpose of the warning was to find accidental
conversions (eg: "const std::pair<int, int> &x : my_map"). I'm not sure
what bugs we would be detecting/avoiding if we used references for cases
where there is no conversion. Are there any particular bugs/problems you
foresee/have encountered with that case?
- David
>
>
>> > or the reference is kept around after the loop.
>> > However, this was the whole point of the warning, to divide up the
>> syntax
>> > for range-based for-loops so that every copy is done with a
>> non-reference
>> > type while every reference type will not bind to a temporary.
>> Exceptions to
>> > this rule just weaken the warning.
>>
>>
>>
>> >>
>> >> >
>> >> >>
>> >> >> > // VMap maps all values and blocks that were just cloned,
>> but
>> >> >> > dead
>> >> >> > // blocks which were pruned will map to nullptr.
>> >> >> > if (!isa<BasicBlock>(MapEntry.first) || MapEntry.second ==
>> >> >> > nullptr)
>> >> >> >
>> >> >> > Modified: llvm/trunk/lib/MC/MCDwarf.cpp
>> >> >> > URL:
>> >> >> >
>> >> >> >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCDwarf.cpp?rev=234963&r1=234962&r2=234963&view=diff
>> >> >> >
>> >> >> >
>> >> >> >
>> ==============================================================================
>> >> >> > --- llvm/trunk/lib/MC/MCDwarf.cpp (original)
>> >> >> > +++ llvm/trunk/lib/MC/MCDwarf.cpp Tue Apr 14 20:21:15 2015
>> >> >> > @@ -803,7 +803,7 @@ static void EmitGenDwarfRanges(MCStreame
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> >
>> MCOS->SwitchSection(context.getObjectFileInfo()->getDwarfRangesSection());
>> >> >> >
>> >> >> > - for (const auto sec : Sections) {
>> >> >> > + for (const auto &sec : Sections) {
>> >> >> >
>> >> >> > MCSymbol *StartSymbol = sec.second.first;
>> >> >> > MCSymbol *EndSymbol = sec.second.second;
>> >> >> >
>> >> >> > Modified: llvm/trunk/lib/Object/COFFObjectFile.cpp
>> >> >> > URL:
>> >> >> >
>> >> >> >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/COFFObjectFile.cpp?rev=234963&r1=234962&r2=234963&view=diff
>> >> >> >
>> >> >> >
>> >> >> >
>> ==============================================================================
>> >> >> > --- llvm/trunk/lib/Object/COFFObjectFile.cpp (original)
>> >> >> > +++ llvm/trunk/lib/Object/COFFObjectFile.cpp Tue Apr 14 20:21:15
>> 2015
>> >> >> > @@ -262,7 +262,7 @@ std::error_code COFFObjectFile::getSymbo
>> >> >> > }
>> >> >> > const section_iterator SecEnd = section_end();
>> >> >> > uint64_t AfterAddr = UnknownAddressOrSize;
>> >> >> > - for (const symbol_iterator &SymbI : symbols()) {
>> >> >> > + for (const symbol_iterator SymbI : symbols()) {
>> >> >>
>> >> >> Drop const if we're dropping ref?
>> >> >
>> >> > See two lines above where the code uses a const iterator. This
>> change
>> >> > follows the current code.
>> >>
>> >> Fair enough
>> >>
>> >> >>
>> >> >>
>> >> >> > section_iterator SecI = SecEnd;
>> >> >> > if (std::error_code EC = SymbI->getSection(SecI))
>> >> >> > return EC;
>> >> >> >
>> >> >> > Modified: llvm/trunk/lib/Target/AArch64/AArch64CollectLOH.cpp
>> >> >> > URL:
>> >> >> >
>> >> >> >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64CollectLOH.cpp?rev=234963&r1=234962&r2=234963&view=diff
>> >> >> >
>> >> >> >
>> >> >> >
>> ==============================================================================
>> >> >> > --- llvm/trunk/lib/Target/AArch64/AArch64CollectLOH.cpp (original)
>> >> >> > +++ llvm/trunk/lib/Target/AArch64/AArch64CollectLOH.cpp Tue Apr 14
>> >> >> > 20:21:15 2015
>> >> >> > @@ -328,7 +328,7 @@ static void initReachingDef(const Machin
>> >> >> > const uint32_t *PreservedRegs = MO.getRegMask();
>> >> >> >
>> >> >> > // Set generated regs.
>> >> >> > - for (const auto Entry : RegToId) {
>> >> >> > + for (const auto &Entry : RegToId) {
>> >> >> > unsigned Reg = Entry.second;
>> >> >> > // Use the global register ID when querying APIs
>> external
>> >> >> > to
>> >> >> > this
>> >> >> > // pass.
>> >> >> >
>> >> >> > Modified: llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp
>> >> >> > URL:
>> >> >> >
>> >> >> >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp?rev=234963&r1=234962&r2=234963&view=diff
>> >> >> >
>> >> >> >
>> >> >> >
>> ==============================================================================
>> >> >> > --- llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp (original)
>> >> >> > +++ llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp Tue Apr 14
>> >> >> > 20:21:15 2015
>> >> >> > @@ -887,7 +887,7 @@ void StructurizeCFG::createFlow() {
>> >> >> > /// no longer dominate all their uses. Not sure if this is really
>> >> >> > nessasary
>> >> >> > void StructurizeCFG::rebuildSSA() {
>> >> >> > SSAUpdater Updater;
>> >> >> > - for (const auto &BB : ParentRegion->blocks())
>> >> >> > + for (auto *BB : ParentRegion->blocks())
>> >> >> > for (BasicBlock::iterator II = BB->begin(), IE = BB->end();
>> >> >> > II != IE; ++II) {
>> >> >> >
>> >> >> >
>> >> >> > Modified: llvm/trunk/tools/llvm-cxxdump/llvm-cxxdump.cpp
>> >> >> > URL:
>> >> >> >
>> >> >> >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-cxxdump/llvm-cxxdump.cpp?rev=234963&r1=234962&r2=234963&view=diff
>> >> >> >
>> >> >> >
>> >> >> >
>> ==============================================================================
>> >> >> > --- llvm/trunk/tools/llvm-cxxdump/llvm-cxxdump.cpp (original)
>> >> >> > +++ llvm/trunk/tools/llvm-cxxdump/llvm-cxxdump.cpp Tue Apr 14
>> >> >> > 20:21:15
>> >> >> > 2015
>> >> >> > @@ -355,7 +355,8 @@ static void dumpCXXData(const ObjectFile
>> >> >> > StringRef SymName = VFTableEntry.second;
>> >> >> > outs() << VFTableName << '[' << Offset << "]: " << SymName <<
>> >> >> > '\n';
>> >> >> > }
>> >> >> > - for (const std::pair<StringRef, ArrayRef<little32_t>> &VBTable
>> :
>> >> >> > VBTables) {
>> >> >> > + for (const std::pair<const StringRef, ArrayRef<little32_t>>
>> >> >> > &VBTable
>> >> >> > :
>> >> >>
>> >> >> Can probably use 'const auto &' here.
>> >> >
>> >> > Fixed in r234974.
>> >> >>
>> >> >>
>> >> >> > + VBTables) {
>> >> >> > StringRef VBTableName = VBTable.first;
>> >> >> > uint32_t Idx = 0;
>> >> >> > for (little32_t Offset : VBTable.second) {
>> >> >> > @@ -363,7 +364,8 @@ static void dumpCXXData(const ObjectFile
>> >> >> > Idx += sizeof(Offset);
>> >> >> > }
>> >> >> > }
>> >> >> > - for (const std::pair<StringRef, CompleteObjectLocator>
>> &COLPair :
>> >> >> > COLs) {
>> >> >> > + for (const std::pair<const StringRef, CompleteObjectLocator>
>> >> >> > &COLPair
>> >> >> > :
>> >> >> > + COLs) {
>> >> >> > StringRef COLName = COLPair.first;
>> >> >> > const CompleteObjectLocator &COL = COLPair.second;
>> >> >> > outs() << COLName << "[IsImageRelative]: " << COL.Data[0] <<
>> >> >> > '\n';
>> >> >> > @@ -373,7 +375,8 @@ static void dumpCXXData(const ObjectFile
>> >> >> > outs() << COLName << "[ClassHierarchyDescriptor]: " <<
>> >> >> > COL.Symbols[1]
>> >> >> > << '\n';
>> >> >> > }
>> >> >> > - for (const std::pair<StringRef, ClassHierarchyDescriptor>
>> &CHDPair
>> >> >> > :
>> >> >> > CHDs) {
>> >> >> > + for (const std::pair<const StringRef, ClassHierarchyDescriptor>
>> >> >> > &CHDPair :
>> >> >> > + CHDs) {
>> >> >> > StringRef CHDName = CHDPair.first;
>> >> >> > const ClassHierarchyDescriptor &CHD = CHDPair.second;
>> >> >> > outs() << CHDName << "[AlwaysZero]: " << CHD.Data[0] << '\n';
>> >> >> > @@ -381,14 +384,14 @@ static void dumpCXXData(const ObjectFile
>> >> >> > outs() << CHDName << "[NumClasses]: " << CHD.Data[2] << '\n';
>> >> >> > outs() << CHDName << "[BaseClassArray]: " << CHD.Symbols[0]
>> <<
>> >> >> > '\n';
>> >> >> > }
>> >> >> > - for (const std::pair<std::pair<StringRef, uint64_t>, StringRef>
>> >> >> > &BCAEntry :
>> >> >> > - BCAEntries) {
>> >> >> > + for (const std::pair<const std::pair<StringRef, uint64_t>,
>> >> >> > StringRef>
>> >> >>
>> >> >> And "const auto &" here too, I suspect.
>> >> >>
>> >> >> > + &BCAEntry : BCAEntries) {
>> >> >> > StringRef BCAName = BCAEntry.first.first;
>> >> >> > uint64_t Offset = BCAEntry.first.second;
>> >> >> > StringRef SymName = BCAEntry.second;
>> >> >> > outs() << BCAName << '[' << Offset << "]: " << SymName <<
>> '\n';
>> >> >> > }
>> >> >> > - for (const std::pair<StringRef, BaseClassDescriptor> &BCDPair :
>> >> >> > BCDs)
>> >> >> > {
>> >> >> > + for (const std::pair<const StringRef, BaseClassDescriptor>
>> >> >> > &BCDPair :
>> >> >> > BCDs) {
>> >> >>
>> >> >> and here
>> >> >>
>> >> >> > StringRef BCDName = BCDPair.first;
>> >> >> > const BaseClassDescriptor &BCD = BCDPair.second;
>> >> >> > outs() << BCDName << "[TypeDescriptor]: " << BCD.Symbols[0]
>> <<
>> >> >> > '\n';
>> >> >> > @@ -400,7 +403,7 @@ static void dumpCXXData(const ObjectFile
>> >> >> > outs() << BCDName << "[ClassHierarchyDescriptor]: " <<
>> >> >> > BCD.Symbols[1]
>> >> >> > << '\n';
>> >> >> > }
>> >> >> > - for (const std::pair<StringRef, TypeDescriptor> &TDPair : TDs)
>> {
>> >> >> > + for (const std::pair<const StringRef, TypeDescriptor> &TDPair :
>> >> >> > TDs)
>> >> >> > {
>> >> >>
>> >> >> and here
>> >> >>
>> >> >> > StringRef TDName = TDPair.first;
>> >> >> > const TypeDescriptor &TD = TDPair.second;
>> >> >> > outs() << TDName << "[VFPtr]: " << TD.Symbols[0] << '\n';
>> >> >> > @@ -410,7 +413,7 @@ static void dumpCXXData(const ObjectFile
>> >> >> > /*UseHexEscapes=*/true)
>> >> >> > << '\n';
>> >> >> > }
>> >> >> > - for (const std::pair<StringRef, ThrowInfo> &TIPair : TIs) {
>> >> >> > + for (const std::pair<const StringRef, ThrowInfo> &TIPair :
>> TIs) {
>> >> >>
>> >> >> and here
>> >> >>
>> >> >> > StringRef TIName = TIPair.first;
>> >> >> > const ThrowInfo &TI = TIPair.second;
>> >> >> > auto dumpThrowInfoFlag = [&](const char *Name, uint32_t
>> Flag) {
>> >> >> > @@ -429,7 +432,7 @@ static void dumpCXXData(const ObjectFile
>> >> >> > dumpThrowInfoSymbol("ForwardCompat", 8);
>> >> >> > dumpThrowInfoSymbol("CatchableTypeArray", 12);
>> >> >> > }
>> >> >> > - for (const std::pair<StringRef, CatchableTypeArray> &CTAPair :
>> >> >> > CTAs)
>> >> >> > {
>> >> >> > + for (const std::pair<const StringRef, CatchableTypeArray>
>> &CTAPair
>> >> >> > :
>> >> >> > CTAs) {
>> >> >>
>> >> >> And here
>> >> >>
>> >> >> > StringRef CTAName = CTAPair.first;
>> >> >> > const CatchableTypeArray &CTA = CTAPair.second;
>> >> >> >
>> >> >> > @@ -441,7 +444,7 @@ static void dumpCXXData(const ObjectFile
>> >> >> > I != E; ++I)
>> >> >> > outs() << CTAName << '[' << Idx++ << "]: " << I->second <<
>> >> >> > '\n';
>> >> >> > }
>> >> >> > - for (const std::pair<StringRef, CatchableType> &CTPair : CTs) {
>> >> >> > + for (const std::pair<const StringRef, CatchableType> &CTPair :
>> >> >> > CTs) {
>> >> >>
>> >> >> And here
>> >> >>
>> >> >> > StringRef CTName = CTPair.first;
>> >> >> > const CatchableType &CT = CTPair.second;
>> >> >> > auto dumpCatchableTypeFlag = [&](const char *Name, uint32_t
>> >> >> > Flag) {
>> >> >> > @@ -464,14 +467,14 @@ static void dumpCXXData(const ObjectFile
>> >> >> > << "[CopyCtor]: " << (CT.Symbols[1].empty() ? "null" :
>> >> >> > CT.Symbols[1])
>> >> >> > << '\n';
>> >> >> > }
>> >> >> > - for (const std::pair<std::pair<StringRef, uint64_t>, StringRef>
>> >> >> > &VTTPair :
>> >> >> > - VTTEntries) {
>> >> >> > + for (const std::pair<const std::pair<StringRef, uint64_t>,
>> >> >> > StringRef>
>> >> >>
>> >> >> And here
>> >> >>
>> >> >> > + &VTTPair : VTTEntries) {
>> >> >> > StringRef VTTName = VTTPair.first.first;
>> >> >> > uint64_t VTTOffset = VTTPair.first.second;
>> >> >> > StringRef VTTEntry = VTTPair.second;
>> >> >> > outs() << VTTName << '[' << VTTOffset << "]: " << VTTEntry <<
>> >> >> > '\n';
>> >> >> > }
>> >> >> > - for (const std::pair<StringRef, StringRef> &TIPair : TINames) {
>> >> >> > + for (const std::pair<const StringRef, StringRef> &TIPair :
>> >> >> > TINames) {
>> >> >>
>> >> >> And here
>> >> >>
>> >> >> > StringRef TIName = TIPair.first;
>> >> >> > outs() << TIName << ": " << TIPair.second << '\n';
>> >> >> > }
>> >> >> >
>> >> >> > Modified: llvm/trunk/utils/TableGen/CodeGenRegisters.cpp
>> >> >> > URL:
>> >> >> >
>> >> >> >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/CodeGenRegisters.cpp?rev=234963&r1=234962&r2=234963&view=diff
>> >> >> >
>> >> >> >
>> >> >> >
>> ==============================================================================
>> >> >> > --- llvm/trunk/utils/TableGen/CodeGenRegisters.cpp (original)
>> >> >> > +++ llvm/trunk/utils/TableGen/CodeGenRegisters.cpp Tue Apr 14
>> >> >> > 20:21:15
>> >> >> > 2015
>> >> >> > @@ -1780,7 +1780,7 @@ void CodeGenRegBank::computeRegUnitLaneM
>> >> >> > const CodeGenRegister *SubRegister = S->second;
>> >> >> > unsigned LaneMask = SubRegIndex->LaneMask;
>> >> >> > // Distribute LaneMask to Register Units touched.
>> >> >> > - for (const auto &SUI : SubRegister->getRegUnits()) {
>> >> >> > + for (unsigned SUI : SubRegister->getRegUnits()) {
>> >> >> > bool Found = false;
>> >> >> > unsigned u = 0;
>> >> >> > for (unsigned RU : RegUnits) {
>> >> >> >
>> >> >> >
>> >> >> > _______________________________________________
>> >> >> > llvm-commits mailing list
>> >> >> > llvm-commits at cs.uiuc.edu
>> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >> >
>> >> >
>> >
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150415/ecaaffac/attachment.html>
More information about the llvm-commits
mailing list