[Lldb-commits] [PATCH] D58410: [Reproducers] Initialize reproducers before initializing the debugger.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this looks mostly fine. See my comment about not using SB classes in the reproducer api. I still kind of like the idea of naming the reproducer class in some special way, to make it more obvious that it is not "just another" SB class, but I'm not sure if that would be just more confusing.



================
Comment at: lldb/include/lldb/API/SBReproducer.h:18-20
   static bool Replay();
+  static lldb::SBError InitializeCapture(const char *path);
+  static lldb::SBError InitializeReplay(const char *path);
----------------
So, in this new world, how are we expected to perform the replay? `InitializeReplay()`, followed by `Replay()` ? Could we fold those two calls into one? I don't see what meaningful work could the user do between those two calls, as all sb calls (including SBDebugger::Initialize) will now be recorded (right?).


================
Comment at: lldb/source/API/SBDebugger.cpp:127
 void SBDebugger::Initialize() {
-  SBInitializerOptions options;
-  SBDebugger::Initialize(options);
+  SBError ignored = SBDebugger::InitializeWithErrorHandling();
 }
----------------
I think the usual way to do that is to just cast the result to void.


================
Comment at: lldb/source/API/SBReproducer.cpp:49
+SBError SBReproducer::InitializeCapture(const char *path) {
+  SBError error;
+  if (auto e = Reproducer::Initialize(ReproducerMode::Capture, FileSpec(path)))
----------------
JDevlieghere wrote:
> The SBError is a problem. We can create it after the initialization, but then we’d need a bogus repro::Record to make sure the api boundary is correctly registered. Let me know if you can think of an alternative.
Maybe this function should not return an SBError at all (just like it does not accept an SBFileSpec due to bootstrapping problems). You could just return a std::string, and have an empty string denote success. This would be consistent with the idea that the repro API is not a part of SB API, but rather something sitting slightly above (or at least, besides) it.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D58410