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:
Change SmallPtrSet to SmallVector for the container (VarDecls) being iterated - this may have a compile time impact (need to measure).
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?
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
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.