[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 6:25 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, May 11, 2015 at 11:36 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> 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,
>
>
> What utility? Clang holds a pretty high bar of "warnings that find bugs" - I
> haven't seen a case where this has found a bug (I think we've fixed about 2
> (3?) cases of this warning so far & all were mechanical transformations).

Put another way, I can find no justification for allowing "this
variable is declared but not used" warnings, regardless of what
compiler identifies the issue. Again, not everyone has the luxury of
bootstrapping, and I would rather not have a bot tell me when I've
screwed something up if my compiler can tell me first. The bots should
be a fallback, not a first line of defense (in cases where the warning
doesn't require us to do design gymnastics that make the code less
maintainable).

> This seems stylistic rather than bug-finding.

This particular case is very much a one-off and represents a bug in
Visual Studio. I do not think it should be held up as a reason to
disable a sensible warning.

>
>>
>> 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.
>
>
> Committed some parts of this in r237059. Didn't manage to un-classify the
> TargetRegistry due to some friendship stuff going on with the Target - left
> a FIXME comment. I don't know why a bunch of those functions aren't just
> members of Target.

Thanks!

~Aaron

>
>>
>>
>> ~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
>> >
>> >
>
>