[PATCH] D151019: [llvm-exegesis] Refactor FunctionExecutorImpl and create factory

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:195
 
+  RC.ExecutionMode = ExecutionMode;
+
----------------
A runnable configuration exists outside of how it's executed, it feels weird to have this here. Why isn't this a member of `BenchmarkRunner` ? 


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:204
+    object::OwningBinary<object::ObjectFile> ObjectFile,
+    const BenchmarkKey &Key) const {
+  if (ExecutionMode == ExecutionModeE::InProcess) {
----------------
Why do we need `Key` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:205
+    const BenchmarkKey &Key) const {
+  if (ExecutionMode == ExecutionModeE::InProcess) {
+    return std::make_unique<InProcessFunctionExecutorImpl>(
----------------
[nit] use switch, even if it's only one option for now, I think it's more readable for enums.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151019