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

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:389
 /// 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 runDataflowReturnError(
+    llvm::StringRef Code,
----------------
gribozavr2 wrote:
> mboehme wrote:
> > gribozavr2 wrote:
> > > 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`.
> > 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?
> 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?
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154339