[clang][dataflow] Make `runDataflowReturnError()` a non-template function.

[mboehme]

Summary

It turns out this didn't need to be a template at all.

Likewise, change callers to they're non-template functions.

Also, correct / clarify some comments in RecordOps.h.

This is in response to post-commit comments on https://reviews.llvm.org/D153006.

[gribozavr2]

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

390–398

I don't quite understand your comment from the previous patch:

This is really just moved from TransferTest.cpp -- and it's more closely related to the runDataflow() functions there and in the newly added RecordOps.cpp. (I can't call it runDataflow() because then it would differ only in return type from one of the functions in the overload set.)

I don't see another checkDataflow() function with the same signature. Furthermore, checkDataflow() overloads above already return an llvm::Error.

[mboehme]

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

390–398

What I meant is: This function seemed to me to be more closely related to the runDataflow() functions in TransferTest.cpp than to the checkDataflow() functions above -- because of the parameter types (the checkDataflow() functions take AnalysisInputs, while this this function doesn't), and also because this function, like the runDataflow() functions, is not a template, while the checkDataflow() functions are.

Should I ignore these superficial similarities?

[gribozavr2]

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

390–398

Ah I think I understand. I'm just trying to make sense of the many overloads we have here.

Okay so since this function hardcodes NoopAnalysis (unlike checkDataflow()) do you think that should be reflected in the name somehow?

[mboehme]

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

390–398

Ah I think I understand. I'm just trying to make sense of the many overloads we have here.

Yup, it's confusing...

Okay so since this function hardcodes NoopAnalysis (unlike checkDataflow()) do you think that should be reflected in the name somehow?

Good point. Maybe this is a path to a good name. How about something like checkDataflowWithNoopAnalysis()? Still a mouthful, but I think it reflects better what this actually does, and it also explains why it's not a template.

[gribozavr2]

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

390–398

SGTM!

[mboehme]

Rename runDataflowReturnError() as discussed in review.

clang/include/clang/Analysis/FlowSensitive/RecordOps.h

 
 namespace clang {
 namespace dataflow {
 
 /// Copies a record (struct, class, or union) from `Src` to `Dst`.
 ///
 /// This performs a deep copy, i.e. it copies every field and recurses on
 /// fields of record type. It also copies properties from the `StructValue`
-/// associated with `Dst` to the `StructValue` associated with `Src` (if these
+/// associated with `Src` to the `StructValue` associated with `Dst` (if these
 /// `StructValue`s exist).
 ///
 /// If there is a `StructValue` associated with `Dst` in the environment, this
 /// function creates a new `StructValue` and associates it with `Dst`; clients
 /// need to be aware of this and must not assume that the `StructValue`
 /// associated with `Dst` remains the same after the call.
 ///
 /// We create a new `StructValue` rather than modifying properties on the old

 /// retrieved from `Env2`. A convenience overload retrieves values for `Loc1`
 /// and `Loc2` from the same environment.
 ///
 /// This performs a deep comparison, i.e. it compares every field and recurses
 /// on fields of record type. Fields of reference type compare equal if they
 /// refer to the same storage location. If `StructValue`s are associated with
 /// `Loc1` and `Loc2`, it also compares the properties on those `StructValue`s.
 ///
+/// Note on how to interpret the result:
+/// - If this returns true, the records are guaranteed to be equal at runtime.
+/// - If this returns false, the records may still be equal at runtime; our
+///   analysis merely cannot guarantee that they will be equal.
+///
 /// Requirements:
 ///
 ///  `Src` and `Dst` must have the same canonical unqualified type.
 bool recordsEqual(const AggregateStorageLocation &Loc1, const Environment &Env1,
                   const AggregateStorageLocation &Loc2,
                   const Environment &Env2);
 
 inline bool recordsEqual(const AggregateStorageLocation &Loc1,

clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp

 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace dataflow {
 namespace test {
 namespace {
 
-template <typename VerifyResultsT>
-void runDataflow(llvm::StringRef Code, VerifyResultsT VerifyResults,
-                 LangStandard::Kind Std = LangStandard::lang_cxx17,
-                 llvm::StringRef TargetFun = "target") {
+void runDataflow(
+    llvm::StringRef Code,
+    std::function<
+        void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &,
+             ASTContext &)>
+        VerifyResults,
   ASSERT_THAT_ERROR(
-      runDataflowReturnError(Code, VerifyResults,
-                             DataflowAnalysisOptions{BuiltinOptions{}}, Std,
-                             TargetFun),
+      checkDataflowWithNoopAnalysis(Code, VerifyResults,
+                                    DataflowAnalysisOptions{BuiltinOptions{}},
+                                    Std, TargetFun),
       llvm::Succeeded());
 }
 
 TEST(RecordOpsTest, CopyRecord) {
   std::string Code = R"(
     struct S {
       int outer_int;
       int &ref;

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

return checkDataflow<AnalysisT>(
         AnnotationStates.clear();
       });
 }
 
 using BuiltinOptions = DataflowAnalysisContext::Options;
 
 /// Runs dataflow on `Code` with a `NoopAnalysis` and calls `VerifyResults` to
 /// verify the results.
-template <typename VerifyResultsT>
-llvm::Error
-runDataflowReturnError(llvm::StringRef Code, VerifyResultsT VerifyResults,
-                       DataflowAnalysisOptions Options,
-                       LangStandard::Kind Std = LangStandard::lang_cxx17,
-                       llvm::StringRef TargetFun = "target") {
-  using ast_matchers::hasName;
-  llvm::SmallVector<std::string, 3> ASTBuildArgs = {
-      // -fnodelayed-template-parsing is the default everywhere but on Windows.
-      // Set it explicitly so that tests behave the same on Windows as on other
-      // platforms.
-      "-fsyntax-only", "-fno-delayed-template-parsing",
-      "-std=" +
-          std::string(LangStandard::getLangStandardForKind(Std).getName())};
-  AnalysisInputs<NoopAnalysis> AI(
-      Code, hasName(TargetFun),
-      [UseBuiltinModel = Options.BuiltinOpts.has_value()](ASTContext &C,
-                                                          Environment &Env) {
-        return NoopAnalysis(
-            C,
-            DataflowAnalysisOptions{
-                UseBuiltinModel ? Env.getDataflowAnalysisContext().getOptions()
-                                : std::optional<BuiltinOptions>()});
-      });
-  AI.ASTBuildArgs = ASTBuildArgs;
-  if (Options.BuiltinOpts)
-    AI.BuiltinOptions = *Options.BuiltinOpts;
-  return checkDataflow<NoopAnalysis>(
-      std::move(AI),
-      /*VerifyResults=*/
-      [&VerifyResults](
-          const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
-          const AnalysisOutputs &AO) { VerifyResults(Results, AO.ASTCtx); });
-}
+llvm::Error checkDataflowWithNoopAnalysis(
+    llvm::StringRef Code,
+    std::function<
+        void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &,
+             ASTContext &)>
+        VerifyResults,
+    llvm::StringRef TargetFun = "target");
 
 /// Returns the `ValueDecl` for the given identifier.
 ///
 /// Requirements:
 ///
 ///   `Name` must be unique in `ASTCtx`.
 const ValueDecl *findValueDecl(ASTContext &ASTCtx, llvm::StringRef Name);
 

clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp

return llvm::createStringError(
             .getLocWithOffset(Annotations[I].Begin)
             .printToString(SourceManager)
             .data());
   }
 
   return Result;
 }
 
+llvm::Error test::checkDataflowWithNoopAnalysis(
+    llvm::StringRef Code,
+    std::function<
+        void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &,
+             ASTContext &)>
+        VerifyResults,
+    DataflowAnalysisOptions Options, LangStandard::Kind Std,
+    llvm::StringRef TargetFun) {
+  using ast_matchers::hasName;
+  llvm::SmallVector<std::string, 3> ASTBuildArgs = {
+      // -fnodelayed-template-parsing is the default everywhere but on Windows.
+      // Set it explicitly so that tests behave the same on Windows as on other
+      // platforms.
+      "-fsyntax-only", "-fno-delayed-template-parsing",
+      "-std=" +
+          std::string(LangStandard::getLangStandardForKind(Std).getName())};
+  AnalysisInputs<NoopAnalysis> AI(
+      Code, hasName(TargetFun),
+      [UseBuiltinModel = Options.BuiltinOpts.has_value()](ASTContext &C,
+                                                          Environment &Env) {
+        return NoopAnalysis(
+            C,
+            DataflowAnalysisOptions{
+                UseBuiltinModel ? Env.getDataflowAnalysisContext().getOptions()
+                                : std::optional<BuiltinOptions>()});
+      });
+  AI.ASTBuildArgs = ASTBuildArgs;
+  if (Options.BuiltinOpts)
+    AI.BuiltinOptions = *Options.BuiltinOpts;
+  return checkDataflow<NoopAnalysis>(
+      std::move(AI),
+      /*VerifyResults=*/
+      [&VerifyResults](
+          const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+          const AnalysisOutputs &AO) { VerifyResults(Results, AO.ASTCtx); });
+}
+
 const ValueDecl *test::findValueDecl(ASTContext &ASTCtx, llvm::StringRef Name) {
   auto TargetNodes = match(
       valueDecl(unless(indirectFieldDecl()), hasName(Name)).bind("v"), ASTCtx);
   assert(TargetNodes.size() == 1 && "Name must be unique");
   auto *const Result = selectFirst<ValueDecl>("v", TargetNodes);
   assert(Result != nullptr);
   return Result;
 }

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

 using namespace dataflow;
 using namespace test;
 using ::testing::Eq;
 using ::testing::IsNull;
 using ::testing::Ne;
 using ::testing::NotNull;
 using ::testing::UnorderedElementsAre;
 
-template <typename VerifyResultsT>
-void runDataflow(llvm::StringRef Code, VerifyResultsT VerifyResults,
-                 DataflowAnalysisOptions Options,
-                 LangStandard::Kind Std = LangStandard::lang_cxx17,
-                 llvm::StringRef TargetFun = "target") {
-  ASSERT_THAT_ERROR(
-      runDataflowReturnError(Code, VerifyResults, Options, Std, TargetFun),
+void runDataflow(
+    llvm::StringRef Code,
+    std::function<
+        void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &,
+             ASTContext &)>
+        VerifyResults,
+  ASSERT_THAT_ERROR(checkDataflowWithNoopAnalysis(Code, VerifyResults, Options,
+                                                  Std, TargetFun),
                     llvm::Succeeded());
 }
 
-template <typename VerifyResultsT>
-void runDataflow(llvm::StringRef Code, VerifyResultsT VerifyResults,
-                 LangStandard::Kind Std = LangStandard::lang_cxx17,
-                 bool ApplyBuiltinTransfer = true,
-                 llvm::StringRef TargetFun = "target") {
+void runDataflow(
+    llvm::StringRef Code,
+    std::function<
+        void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &,
+             ASTContext &)>
+        VerifyResults,
+    bool ApplyBuiltinTransfer = true, llvm::StringRef TargetFun = "target") {
   runDataflow(Code, std::move(VerifyResults),
               {ApplyBuiltinTransfer ? BuiltinOptions{}
                                     : std::optional<BuiltinOptions>()},
               Std, TargetFun);
 }
 
 TEST(TransferTest, IntVarDeclNotTrackedWhenTransferDisabled) {
   std::string Code = R"(

 }
 
 TEST(TransferTest, CannotAnalyzeFunctionTemplate) {
   std::string Code = R"(
     template <typename T>
     void target() {}
   )";
   ASSERT_THAT_ERROR(
-      runDataflowReturnError(
+      checkDataflowWithNoopAnalysis(
           Code,
           [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
              ASTContext &ASTCtx) {},
           {BuiltinOptions()}),
       llvm::FailedWithMessage("Cannot analyze templated declarations"));
 }
 
 TEST(TransferTest, CannotAnalyzeMethodOfClassTemplate) {
   std::string Code = R"(
     template <typename T>
     struct A {
       void target() {}
     };
   )";
   ASSERT_THAT_ERROR(
-      runDataflowReturnError(
+      checkDataflowWithNoopAnalysis(
           Code,
           [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
              ASTContext &ASTCtx) {},
           {BuiltinOptions()}),
       llvm::FailedWithMessage("Cannot analyze templated declarations"));
 }
 
 TEST(TransferTest, VarDeclInitAssignConditionalOperator) {