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

[David Blaikie]

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).
This seems stylistic rather than bug-finding.


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


>
> ~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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150511/c42e7c11/attachment.html>