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

[David Blaikie]

On Mon, Aug 18, 2014 at 7:55 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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.

Can you manage that? I'm not sure what the warning name/number was
that you saw. It /looks/ like the disabling can be done in
cmake/modules/HandleLLVMOptions.cmake around lines 240-250.