[llvm] r236990 - Replacing a range-based for loop with an old-style for loop. This code was previously causing a warning with MSVC about a compiler-generated local variable because TargetRegistry::begin() and end() are static member functions. NFC.

[Aaron Ballman]

On Mon, May 11, 2015 at 2:30 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, May 11, 2015 at 6:10 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> Author: aaronballman
>> Date: Mon May 11 08:10:17 2015
>> New Revision: 236990
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=236990&view=rev
>> Log:
>> Replacing a range-based for loop with an old-style for loop. This code was
>> previously causing a warning with MSVC about a compiler-generated local
>> variable because TargetRegistry::begin() and end() are static member
>> functions. NFC.
>
>
> (usual response from me: I'm not sure this really improves the readability
> of the code, could we just disable the MSVC warning instead?

I think the original code was questionable as well. While it's legal
to access a static member using the dot operator, it's rather
unconventional. ;-)

FWIW, I've also reported the bug to Microsoft:
https://connect.microsoft.com/VisualStudio/feedback/details/1322808/warning-w4-with-compiler-generated-local-variable

I think disabling the warning is, as usual, killing a fly with a
shotgun. The warning provides utility, and this is not a case that is
likely to come up with proper API design.

> Possible alternative: TargetRegistry should probably just be a namespace,
> not a class (it's just a collection of static functions, right?) and
> possibly have some kind of range accessor that returns an object: "for
> (const auto &Target : TargetRegistry::targets())" or the like)

I think that is a good solution, but don't have the time to make such
a change myself.

~Aaron

>
>>
>>
>> Modified:
>>     llvm/trunk/unittests/Support/TargetRegistry.cpp
>>
>> Modified: llvm/trunk/unittests/Support/TargetRegistry.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/TargetRegistry.cpp?rev=236990&r1=236989&r2=236990&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/unittests/Support/TargetRegistry.cpp (original)
>> +++ llvm/trunk/unittests/Support/TargetRegistry.cpp Mon May 11 08:10:17
>> 2015
>> @@ -23,8 +23,9 @@ TEST(TargetRegistry, TargetHasArchType)
>>    llvm::InitializeAllTargetInfos();
>>
>>    llvm::TargetRegistry RegistryRoot;
>> -  for (const auto &Target : RegistryRoot) {
>> -    StringRef Name = Target.getName();
>> +  for (auto &I = TargetRegistry::begin(), &E = TargetRegistry::end();
>> +       I != E; ++I) {
>> +    StringRef Name = I->getName();
>>      // There is really no way (at present) to ask a Target whether it
>> targets
>>      // a specific architecture, because the logic for that is buried in a
>>      // predicate.
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>