<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 11, 2015 at 11:36 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On Mon, May 11, 2015 at 2:30 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, May 11, 2015 at 6:10 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> Author: aaronballman<br>
>> Date: Mon May 11 08:10:17 2015<br>
>> New Revision: 236990<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=236990&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=236990&view=rev</a><br>
>> Log:<br>
>> Replacing a range-based for loop with an old-style for loop. This code was<br>
>> previously causing a warning with MSVC about a compiler-generated local<br>
>> variable because TargetRegistry::begin() and end() are static member<br>
>> functions. NFC.<br>
><br>
><br>
> (usual response from me: I'm not sure this really improves the readability<br>
> of the code, could we just disable the MSVC warning instead?<br>
<br>
</span>I think the original code was questionable as well. While it's legal<br>
to access a static member using the dot operator, it's rather<br>
unconventional. ;-)<br>
<br>
FWIW, I've also reported the bug to Microsoft:<br>
<a href="https://connect.microsoft.com/VisualStudio/feedback/details/1322808/warning-w4-with-compiler-generated-local-variable" target="_blank">https://connect.microsoft.com/VisualStudio/feedback/details/1322808/warning-w4-with-compiler-generated-local-variable</a><br>
<br>
I think disabling the warning is, as usual, killing a fly with a<br>
shotgun. The warning provides utility, </blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">and this is not a case that is likely to come up with proper API design.<br>
<span><br>
> Possible alternative: TargetRegistry should probably just be a namespace,<br>
> not a class (it's just a collection of static functions, right?) and<br>
> possibly have some kind of range accessor that returns an object: "for<br>
> (const auto &Target : TargetRegistry::targets())" or the like)<br>
<br>
</span>I think that is a good solution, but don't have the time to make such<br>
a change myself.<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><font color="#888888"><br>
~Aaron<br>
</font></span><div><div><br>
><br>
>><br>
>><br>
>> Modified:<br>
>>     llvm/trunk/unittests/Support/TargetRegistry.cpp<br>
>><br>
>> Modified: llvm/trunk/unittests/Support/TargetRegistry.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/TargetRegistry.cpp?rev=236990&r1=236989&r2=236990&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/TargetRegistry.cpp?rev=236990&r1=236989&r2=236990&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/unittests/Support/TargetRegistry.cpp (original)<br>
>> +++ llvm/trunk/unittests/Support/TargetRegistry.cpp Mon May 11 08:10:17<br>
>> 2015<br>
>> @@ -23,8 +23,9 @@ TEST(TargetRegistry, TargetHasArchType)<br>
>>    llvm::InitializeAllTargetInfos();<br>
>><br>
>>    llvm::TargetRegistry RegistryRoot;<br>
>> -  for (const auto &Target : RegistryRoot) {<br>
>> -    StringRef Name = Target.getName();<br>
>> +  for (auto &I = TargetRegistry::begin(), &E = TargetRegistry::end();<br>
>> +       I != E; ++I) {<br>
>> +    StringRef Name = I->getName();<br>
>>      // There is really no way (at present) to ask a Target whether it<br>
>> targets<br>
>>      // a specific architecture, because the logic for that is buried in a<br>
>>      // predicate.<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>