llvm dev - Jun 2017 - [SemaCXX] Should we fix test failing due to reverse iteration?

I see that the following test fails if reverse iteration of SmallPtrSet 
is enabled:

/clang/test/SemaCXX/warn-loop-analysis.cpp/

This is because in SemaStmt.cpp we iterate SmallPtrSet and output 
warnings about the variables not used in the loop.

Expected output: /warning: variables 'i', 'j', and 'k'
used in loop
condition not modified/

Output with reverse iteration: /warning: variables 'k', 'j', and
'i'
used in loop condition not/

I would like the community's opinion on whether this is something worth 
fixing? In this case, should the output always be the same irrespective 
of the iteration order?

If yes, then we have 2 alternatives:

1. Change SmallPtrSet to SmallVector for the container (VarDecls) being 
iterated - this may have a compile time impact (need to measure).

2. Sort the container (VarDecls) before iteration. We can sort based on 
decl source location and decl name. Not sure if these guaranteed to be 
unique?

Thanks,

Mandeep

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170601/8a140647/attachment.html>

Adding cfe-dev

~Craig

On Thu, Jun 1, 2017 at 1:46 PM, Grang, Mandeep Singh via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I see that the following test fails if reverse iteration of SmallPtrSet is
> enabled:
>
> *clang/test/SemaCXX/warn-loop-analysis.cpp*
>
> This is because in SemaStmt.cpp we iterate SmallPtrSet and output warnings
> about the variables not used in the loop.
>
> Expected output: *warning: variables 'i', 'j', and
'k' used in loop
> condition not modified*
>
> Output with reverse iteration: *warning: variables 'k',
'j', and 'i' used
> in loop condition not*
>
> I would like the community's opinion on whether this is something worth
> fixing? In this case, should the output always be the same irrespective of
> the iteration order?
>
> If yes, then we have 2 alternatives:
>
> 1. Change SmallPtrSet to SmallVector for the container (VarDecls) being
> iterated - this may have a compile time impact (need to measure).
>
> 2. Sort the container (VarDecls) before iteration. We can sort based on
> decl source location and decl name. Not sure if these guaranteed to be
> unique?
>
> Thanks,
>
> Mandeep
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170601/4ac41a53/attachment.html>

> On Jun 1, 2017, at 1:49 PM, Craig Topper via cfe-dev <cfe-dev at
lists.llvm.org> wrote:
> 
> Adding cfe-dev
> 
> ~Craig
> 
> On Thu, Jun 1, 2017 at 1:46 PM, Grang, Mandeep Singh via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
> I see that the following test fails if reverse iteration of SmallPtrSet is
enabled:
> 
> clang/test/SemaCXX/warn-loop-analysis.cpp
> 
I think I saw a bot complaining about this. Could you back out the change until
these failures can be addressed? (Apologies if you already have.)
> This is because in SemaStmt.cpp we iterate SmallPtrSet and output warnings
about the variables not used in the loop.
> Expected output: warning: variables 'i', 'j', and
'k' used in loop condition not modified
> 
> Output with reverse iteration: warning: variables 'k', 'j',
and 'i' used in loop condition not
> 
> I would like the community's opinion on whether this is something worth
fixing? In this case, should the output always be the same irrespective of the
iteration order?
> 
Yes, I think so. Running the compiler twice should produce identical
diagnostics.
> If yes, then we have 2 alternatives:
> 
> 1. Change SmallPtrSet to SmallVector for the container (VarDecls) being
iterated - this may have a compile time impact (need to measure).
> 
> 2. Sort the container (VarDecls) before iteration. We can sort based on
decl source location and decl name. Not sure if these guaranteed to be unique?
> 
CheckForLoopConditionalStatement is hot, relative to the code that actually
emits the diagnostic [1]. So I'd prefer the second option.

[1]
http://lab.llvm.org:8080/coverage/coverage-reports/clang/coverage/Users/buildslave/jenkins/sharedspace/clang-stage2-coverage-R
at 2/llvm/tools/clang/lib/Sema/SemaStmt.cpp.html#L1454
<http://lab.llvm.org:8080/coverage/coverage-reports/clang/coverage/Users/buildslave/jenkins/sharedspace/clang-stage2-coverage-R
at 2/llvm/tools/clang/lib/Sema/SemaStmt.cpp.html#L1454>

vedant
> Thanks,
> 
> Mandeep
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> 
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170601/33410569/attachment.html>

On 1 June 2017 at 13:49, Craig Topper via llvm-dev <llvm-dev at
lists.llvm.org>
wrote:
> Adding cfe-dev
>
> ~Craig
>
> On Thu, Jun 1, 2017 at 1:46 PM, Grang, Mandeep Singh via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> I see that the following test fails if reverse iteration of SmallPtrSet
>> is enabled:
>>
>> *clang/test/SemaCXX/warn-loop-analysis.cpp*
>>
>> This is because in SemaStmt.cpp we iterate SmallPtrSet and output
>> warnings about the variables not used in the loop.
>>
>> Expected output: *warning: variables 'i', 'j', and
'k' used in loop
>> condition not modified*
>>
>> Output with reverse iteration: *warning: variables 'k',
'j', and 'i'
>> used in loop condition not*
>>
>> I would like the community's opinion on whether this is something
worth
>> fixing? In this case, should the output always be the same irrespective
of
>> the iteration order?
>>
> Yes. Clang is intended to be fully deterministic, and this covers our
diagnostic output as well as the object code we produce.
> If yes, then we have 2 alternatives:
>>
>> 1. Change SmallPtrSet to SmallVector for the container (VarDecls) being
>> iterated - this may have a compile time impact (need to measure).
>>
>> 2. Sort the container (VarDecls) before iteration. We can sort based on
>> decl source location and decl name. Not sure if these guaranteed to be
>> unique?
>>
> LLVM provides order-preserving containers for situations such as this. We
should probably use an llvm::SmallSetVector here.
> Thanks,
>>
>> Mandeep
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170601/c473a14c/attachment.html>

This is my old code.  I went ahead and fixed it in r304519.  Let me know if
there's any other trouble from it.

(To the mailing lists this time.)

On Thu, Jun 1, 2017 at 1:46 PM, Grang, Mandeep Singh via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I see that the following test fails if reverse iteration of SmallPtrSet is
> enabled:
>
> *clang/test/SemaCXX/warn-loop-analysis.cpp*
>
> This is because in SemaStmt.cpp we iterate SmallPtrSet and output warnings
> about the variables not used in the loop.
>
> Expected output: *warning: variables 'i', 'j', and
'k' used in loop
> condition not modified*
>
> Output with reverse iteration: *warning: variables 'k',
'j', and 'i' used
> in loop condition not*
>
> I would like the community's opinion on whether this is something worth
> fixing? In this case, should the output always be the same irrespective of
> the iteration order?
>
> If yes, then we have 2 alternatives:
>
> 1. Change SmallPtrSet to SmallVector for the container (VarDecls) being
> iterated - this may have a compile time impact (need to measure).
>
> 2. Sort the container (VarDecls) before iteration. We can sort based on
> decl source location and decl name. Not sure if these guaranteed to be
> unique?
>
> Thanks,
>
> Mandeep
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170601/7f8612a5/attachment.html>

Thanks for the fix, Richard.

On 6/1/2017 9:41 PM, Richard Trieu wrote:
> This is my old code.  I went ahead and fixed it in r304519.  Let me 
> know if there's any other trouble from it.
>
> (To the mailing lists this time.)
>
> On Thu, Jun 1, 2017 at 1:46 PM, Grang, Mandeep Singh via llvm-dev 
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>
>     I see that the following test fails if reverse iteration of
>     SmallPtrSet is enabled:
>
>     /clang/test/SemaCXX/warn-loop-analysis.cpp/
>
>     This is because in SemaStmt.cpp we iterate SmallPtrSet and output
>     warnings about the variables not used in the loop.
>
>     Expected output: /warning: variables 'i', 'j', and
'k' used in
>     loop condition not modified/
>
>     Output with reverse iteration: /warning: variables 'k',
'j', and
>     'i' used in loop condition not/
>
>     I would like the community's opinion on whether this is something
>     worth fixing? In this case, should the output always be the same
>     irrespective of the iteration order?
>
>     If yes, then we have 2 alternatives:
>
>     1. Change SmallPtrSet to SmallVector for the container (VarDecls)
>     being iterated - this may have a compile time impact (need to
>     measure).
>
>     2. Sort the container (VarDecls) before iteration. We can sort
>     based on decl source location and decl name. Not sure if these
>     guaranteed to be unique?
>
>     Thanks,
>
>     Mandeep
>
>
>     _______________________________________________
>     LLVM Developers mailing list
>     llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>     <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170602/acf928e5/attachment.html>