[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

Adding cfe-dev

Adding cfe-dev

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

Yes, I think so. Running the compiler twice should produce identical diagnostics.

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

vedant

Adding cfe-dev

~Craig

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.

Adding cfe-dev

~Craig

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

Nice trick checking the coverage for a quick hotness check!

-- Sean Silva

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

Thanks for the fix, Richard.