martong marked 13 inline comments as done. martong added a comment. In D136668#3883241 <https://reviews.llvm.org/D136668#3883241>, @ymandel wrote:
> Nice!! Just nits. > > At a high level, I'm a little concerned about this as a demo, since I > wouldn't recommend this implementation in practice (for various reasons, > e.g. I would encode with 2 booleans since there are only 3 values). But, I > think this approach has the advantage of clarity over optimized approaches. > It might be worth pointing this out in comments at places where you made a > decision for purposes of clarity/readability, if any come to mind. Okay, thanks again for the review! I've changed the comments for the file, where I describe these reasons about using 3 booleans. ================ Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:183 + break; + case BO_NE: // Noop. + break; ---------------- ymandel wrote: > why not? its a test, so not necessary, just curious to know why you're > leaving this out. This was my thought process: There is nothing that we can say about `b` if `a` is positive. Because `b` can still be either negative, positive, or even zero. Similar reasoning goes to the case when `a` is negative. However, we could have an implication if `a` is zero, namely that `b` is either positive or negative. But I think implications with `or` are not useful, they should be then rather expressed in the lattice. So, we should rather have a new boolean value like PosOrNeg, but I've found that too complicated for this demo. ================ Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:396 + +// FIXME add this to testing support. +template <typename NodeType, typename MatcherType> ---------------- ymandel wrote: > It doesn't appear to be used. Yes, good catch, it's been left here for some older version of the implementation. I removed it now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136668/new/ https://reviews.llvm.org/D136668 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
