Grang, Mandeep Singh via llvm-dev
2017-Jun-01 20:46 UTC
[llvm-dev] [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>
Craig Topper via llvm-dev
2017-Jun-01 20:49 UTC
[llvm-dev] [SemaCXX] Should we fix test failing due to reverse iteration?
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>
Vedant Kumar via llvm-dev
2017-Jun-01 22:49 UTC
[llvm-dev] [cfe-dev] [SemaCXX] Should we fix test failing due to reverse iteration?
> 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>
Richard Smith via llvm-dev
2017-Jun-01 22:50 UTC
[llvm-dev] [SemaCXX] Should we fix test failing due to reverse iteration?
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 ourdiagnostic 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. Weshould 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>
Richard Trieu via llvm-dev
2017-Jun-02 04:41 UTC
[llvm-dev] [SemaCXX] Should we fix test failing due to reverse iteration?
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>
Grang, Mandeep Singh via llvm-dev
2017-Jun-02 07:16 UTC
[llvm-dev] [SemaCXX] Should we fix test failing due to reverse iteration?
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>