[PATCH] D121797: [clang][dataflow] Add modeling of Chromium's CHECK functionality

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Models/ChromiumCheckModel.h:15
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
----------------
This is unnecessary.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/ChromiumCheckModel.cpp:49-55
+  if (const auto *Call = dyn_cast<CallExpr>(Stmt))
+    if (const auto *M = dyn_cast<CXXMethodDecl>(Call->getDirectCallee()))
+      if (M->isStatic() && isCheckLikeMethod(CheckDecls, *M)) {
+        // Logically, mark this branch as unreachable.
+        Env.addToFlowCondition(Env.getBoolLiteralValue(false));
+        return true;
+      }
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/Models/ChromiumCheckModel.cpp:51
+    if (const auto *M = dyn_cast<CXXMethodDecl>(Call->getDirectCallee()))
+      if (M->isStatic() && isCheckLikeMethod(CheckDecls, *M)) {
+        // Logically, mark this branch as unreachable.
----------------
Shouldn't this be part of `isCheckLikeMethod`?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/ChromiumCheckModel.cpp:52
+      if (M->isStatic() && isCheckLikeMethod(CheckDecls, *M)) {
+        // Logically, mark this branch as unreachable.
+        Env.addToFlowCondition(Env.getBoolLiteralValue(false));
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/Models/ChromiumCheckModel.cpp:57
+  return false;
+}
+} // namespace dataflow
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp:90
+
+static constexpr char BadChromiumCheckHeader[] = R"(
+namespace other {
----------------
Perhaps "Incorrect" instead of "Bad" and comment on what makes it incorrect?


================
Comment at: clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp:133
+  void runDataflow(llvm::StringRef Code, Matcher Match,
+                   LangStandard::Kind Std = LangStandard::lang_cxx17) {
+    const tooling::FileContentMappings FileContents = {
----------------
We're not testing with other standards so remove this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121797/new/

https://reviews.llvm.org/D121797