[PATCH] D62611: [analyzer][Dominators] Add unittests
Gábor Horváth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 5 02:20:43 PDT 2019
xazax.hun added a comment.
Oh, I realized later that code I commented on were only moved from somewhere else. If you feel like tackling these comments feel free to do so in a separate patch so this one stays clean (no changes to moved code).
================
Comment at: clang/unittests/Analysis/CFGBuilder.h:18
+public:
+ enum Status {
+ ToolFailed,
----------------
Is it actually sensible to write tests where the tool failed? I can imagine these status codes being helpful for debugging but they has little to do with the tests. I wonder if we actually need all these or a nullable pointer as a result is sufficient.
================
Comment at: clang/unittests/Analysis/CFGBuilder.h:54
+
+inline BuildResult BuildCFG(const char *Code) {
+ CFGCallback Callback;
----------------
Do you expect the code to only contain one function? If so, you should enforce it.
================
Comment at: clang/unittests/Analysis/CFGBuilder.h:65
+ return BuildResult::ToolFailed;
+ return std::move(Callback.TheBuildResult);
+}
----------------
Do you need the std::move here?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62611/new/
https://reviews.llvm.org/D62611
More information about the cfe-commits
mailing list