ymandel marked 2 inline comments as done.
ymandel added a comment.
Thanks for the great comments and the fast response time!
================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:134
+
+ transferBlock(
+ BlockStates, *Block, Env, Analysis,
----------------
I've inlined `buildAnnotationToOutputStateMapping`, since it is only intended
for use here anyhow and factoring it out didn't save anything because we still
need to translate to typed state.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:48
+ << std::declval<const Lattice &>())>
+std::ostream &operator<<(std::ostream &OS,
+ const DataflowAnalysisState<Lattice> &S) {
----------------
xazax.hun wrote:
> This would also be useful for debugging. I wonder whether utilities not
> strictly related to testing should be closer to the actual code, so someone
> wanting to use this for debugging does not need to include the header that is
> dedicated to the tests.
Agreed. Added a FIXME.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:77
+// the files provided in `VirtualMappedFiles`.
+bool runOnCode(llvm::StringRef Code,
+ const std::function<void(ASTContext &)> &Operation,
----------------
xazax.hun wrote:
> I feel like some of our tests keep recreating lightweight versions of the
> LibTooling. Not really an action item for this PR, just a rant :) I hope we
> will have some more centralized stuff at some point.
No, I agree! I missed this -- there's no good reason for runOnCode to exist.
I've removed it and like the result a lot more. :)
================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:94
+template <typename AnalysisT>
+void runDataflow(
+ llvm::StringRef Code,
----------------
xazax.hun wrote:
> Since this function is actually matching the dataflow results against
> expectations, I wonder if something like `checkDataflow` would better
> describe its function. But feel free to keep the current name.
Agreed.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:137
+ TypeErasedBlockStates =
+ runTypeErasedDataflowAnalysis(*cfg, Analysis, Env);
+
----------------
xazax.hun wrote:
> Wouldn't users end up calling the template (not the type erased) version of
> this function? I wonder if we should mimic how users would interact with the
> framework in the tests.
I see your point and tried to move in that direction. However, it makes this
code a lot messier because of the need to iterate again over each block with
`transferBlock` to recoved statement level state. Since `transferBlock` is part
of the type-erased engine, interacting with it would require mapping all of the
state to untyped before using it, which somewhat defeats the whole purpose of
using the typed version.
We should consider reporting statement level information from our
`run...DataflowAnalysis` functions, which would make this iteration
unnecessary, at which point moving to the typed version would make a lot of
sense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115341/new/
https://reviews.llvm.org/D115341
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
