vabridgers wrote:
> I wish we had something like this. It might be possible, but really really
> challenging to implement. To me the problem is that the iteration or lookups
> are not necessarily bad. And to determine if it's bad one needs to understand
> how the result of the lookup or the individual elements of the iteration are
> used.
>
> For example, this is completely fine and efficient:
>
> ```c++
> int numInterestingExprs = 0; // TODO: Use accumulate.
> for (const Expr *E : MyBlocks/*some unordered set-like container*/) {
> if (isInteresting(E)) ++numInterestingExprs;
> }
> ```
>
> But this is not fine, because we apply a sideffect that is dependent on the
> nondeterministic iteration order. In some context, this could be still fine
> btw. It depends on what your constraints are:
>
> ```c++
> for (const Expr *E : MyBlocks/*some unordered set-like container*/) {
> if (isInteresting(E)) llvm::errs() << E->printPretty() << "\n"; // eeeh,
> the lines may get reshuffled from run-to-run...
> }
> ```
>
> Only looking at the for loop here is not enough to decide this.
>
> Limiting the check to only trigger if it sees the iteration AND also the
> use-site - like in the case you match std algorithms, like `is_sorted` or
> `partition` is a much more careful approach. It should work unless the
> algorithm takes a callback/functor that we can't really reason about and we
> could end up with a FP.
>
> Remember that `begin` might be an ADL free-function, and not a memer
> function, such as for `int *arr[10]`. C++ also has `std::ranges::*`, which we
> may wanna cover too in the future.
>
> You mentioned that you evaluated the checker at scale. What was the result of
> the evaluation? I assume you have seen FPs, and TPs as well.
Hi @steakhal , I completed a test run on the LLVM source base and detected no
issues with this check. I will be expanding the tests, and wanted to mention I
just found
> I wish we had something like this. It might be possible, but really really
> challenging to implement. To me the problem is that the iteration or lookups
> are not necessarily bad. And to determine if it's bad one needs to understand
> how the result of the lookup or the individual elements of the iteration are
> used.
>
> For example, this is completely fine and efficient:
>
> ```c++
> int numInterestingExprs = 0; // TODO: Use accumulate.
> for (const Expr *E : MyBlocks/*some unordered set-like container*/) {
> if (isInteresting(E)) ++numInterestingExprs;
> }
> ```
>
> But this is not fine, because we apply a sideffect that is dependent on the
> nondeterministic iteration order. In some context, this could be still fine
> btw. It depends on what your constraints are:
>
> ```c++
> for (const Expr *E : MyBlocks/*some unordered set-like container*/) {
> if (isInteresting(E)) llvm::errs() << E->printPretty() << "\n"; // eeeh,
> the lines may get reshuffled from run-to-run...
> }
> ```
>
> Only looking at the for loop here is not enough to decide this.
>
> Limiting the check to only trigger if it sees the iteration AND also the
> use-site - like in the case you match std algorithms, like `is_sorted` or
> `partition` is a much more careful approach. It should work unless the
> algorithm takes a callback/functor that we can't really reason about and we
> could end up with a FP.
>
> Remember that `begin` might be an ADL free-function, and not a memer
> function, such as for `int *arr[10]`. C++ also has `std::ranges::*`, which we
> may wanna cover too in the future.
>
> You mentioned that you evaluated the checker at scale. What was the result of
> the evaluation? I assume you have seen FPs, and TPs as well.
I completed an initial test run on the llvm/clang repo with no crashes or
results at all. Meaning no FPs or TPs. I will be expanding the tests to more
repos, and would love to hear suggestions if you have any. I suppose abseil
would be a good one. Also just found @Xazax-hun 's testbench to help with this
here - https://github.com/Xazax-hun/csa-testbench. Really looking forward to
trying that :)
Thanks for the comments. Will keep driving this forward.
https://github.com/llvm/llvm-project/pull/110471
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
