dkrupp marked an inline comment as done.
dkrupp added a comment.
@steakhal thanks for your review. All your remarks have been fixed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Taint.cpp:147-150
bool taint::isTainted(ProgramStateRef State, const Stmt *S,
const LocationContext *LCtx, TaintTagType Kind) {
- SVal val = State->getSVal(S, LCtx);
- return isTainted(State, val, Kind);
+ return !getTaintedSymbols(State, S, LCtx, Kind).empty();
}
----------------
steakhal wrote:
> TBH I'm not sure if I like that now we allocate unbounded amount of times
> (because of `getTaintedSymbols()` is recursive and returns by value), where
> we previously did not.
>
> What we could possibly do is to compute the elements of this sequence lazily.
> I'm thinking of the `llvm::mapped_iterator`, but I'm not sure if it's
> possible to have something like that as a return type as it might encode the
> map function in the type or something like that.
> Anyway, I'm just saying that it would be nice to not do more than it's
> necessary, and especially not allocate a lot of short-lived objects there.
>
> Do you think there is a way to have the cake and eat it too?
>
> ---
>
> I did some investigation, and one could get pretty far in the implementation,
> and maybe even complete it but it would be really complicated as of now.
> Maybe we could revisit this subject when we have coroutines.
>
> So, I would suggest to have two sets of APIs:
> - the usual `isTainted(.) -> bool`
> - and a `getTaintedSymbols(.) -> vector<Sym>`
> The important point would be that the `isTainted()` version would not eagerly
> collect all tainted sub-syms but return on finding the first one.
> While, the `getTaintedSymbols()` would collect eagerly all of them, as its
> name suggests.
>
> Imagine if `getTaintedSymbolsImpl()` had an extra flag like `bool
> returnAfterFirstMatch`. This way `isTainted()` can call it like that. While
> in the other case, the parameter would be `false`, and eagerly collect all
> symbols.
>
> This is probably the best of both worlds, as it prevents `isTainted` from
> doing extra work and if we need to iterate over the tainted symbols, we
> always iterate over all of them, so doing it lazily wouldn't gain us much in
> that case anyway.
> As a bonus, the user-facing API would be self-descriptive.
>
> WDYT?
Good idea. I implemented the early return option in getTaintedSymbols(). This
is used now by the isTainted() function.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144269/new/
https://reviews.llvm.org/D144269
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
