[llvm] r215888 - Silencing an MSVC warning about loop variable conflicting with a variable from an outer scope. NFC.

[Aaron Ballman]

On Mon, Aug 18, 2014 at 10:46 AM, David Blaikie <dblaikie at gmail.com> wrote:
> On Mon, Aug 18, 2014 at 7:33 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> On Mon, Aug 18, 2014 at 10:18 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Mon, Aug 18, 2014 at 4:51 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>> Author: aaronballman
>>>> Date: Mon Aug 18 06:51:41 2014
>>>> New Revision: 215888
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=215888&view=rev
>>>> Log:
>>>> Silencing an MSVC warning about loop variable conflicting with a variable from an outer scope. NFC.
>>>
>>> Sorry if this gets a bit repetitious (I ask this about every non-clang
>>> warning we make code fixes for) - should we just disable this MSVC
>>> warning in LLVM's build system?
>>
>> I don't think we should; MSVC only seems to warn on code that is truly
>> strange. In this case, the loop variable was unsigned, but the
>> outer-scope variable of type SDNode * was being used within the loop.
>
> I'm not sure I follow - there's no mention of "N" inside the loop that
> would appear to be using the outer-scope variable (how could it -
> scoping rules would pick the closest scoped "N")?

Sorry, I misspoke (oye...Monday).

You're right, N is not inside of that loop. This is a much less
helpful instance of that warning. It's complaining about a usage of N
immediately *after* the loop.

>
>>> Either that, or we could turn on Clang's -Wshadow to catch these in
>>> clang builds too, if it's a thing we want to catch/fix.
>>
>> If Clang isn't overly noisy, I think I would prefer that.
>
> It looks like even the case you just fixed would meet Clang's bar for
> "false positive" - I don't see the bug here, nor why this case of
> shadowing is particularly more buggy than any other (correct) case.
> The "N" accessed looks like it's entirely the one the author intended.

Yes, this is a false positive. Looking at it again, I'm not entirely
certain of what the point to this warning is. So yes, I think this one
can get turned off.

~Aaron