wyt added inline comments.
================ Comment at: clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp:137-140 + auto CS = E->getAs<CFGStmt>(); + if (!CS) + return; + auto S = CS->getStmt(); ---------------- martong wrote: > This is exactly the same code that you have in > `SingleVarConstantPropagationTest.cpp` and in the rest of the files. I think > this could be hoisted into a common function declared in > `FlowSensitive/DataflowAnalysis.h`. > > By the way, wouldn't it be more convenient for the client to provide a > `transferStmt` function if the they are interested only in `Stmt`s? (Defining > both `transferStmt` and `transfer` should be a compile time error) Thanks for the comment! For now, I decided not to pull this out into a common function. This seems more like a utility function that does not quite belong DataflowAnalysis.h - it would be nice to keep the code there dedicated to analysis logic. Besides, I don't think we save much since the boilerplate is just a few lines. Also, about having another `transferStmt` function, at this stage I feel that it's best if we keep things compact and have just one way of doing things - the framework is constantly evolving and having multiple implementations of the same concept can be distracting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133931/new/ https://reviews.llvm.org/D133931 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
