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

[Sean Silva via cfe-dev]

On Thu, Jun 1, 2017 at 3:49 PM, Vedant Kumar via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

>
> 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> 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
>

Nice trick checking the coverage for a quick hotness check!

-- Sean Silva


>
> vedant
>
> Thanks,
>>
>> Mandeep
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> 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
>
>
>
> _______________________________________________
> 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/2d82a751/attachment.html>