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

[Vedant Kumar via cfe-dev]

> 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@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@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/cfe-dev/attachments/20170601/33410569/attachment.html>