[Lldb-commits] [PATCH] D68434: SBFile support in SBCommandReturnObject

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/include/lldb/Utility/ReproducerInstrumentation.h:257-259
+template <> struct serializer_tag<lldb::SBFile> {
+  typedef NotImplementedTag type;
+};
----------------
This still doesn't seem right to me. Though you have removed the direct `#include`, you only managed to do that since you've forward-declared the class manually -- which I'd consider "cheating". If this is indeed the right solution (which I am not convinced of yet), then this specialization should be somewhere in the API folder (SBFile.cpp, most likely)


================
Comment at: lldb/source/API/SBFile.cpp:115
+
+  R.Register<SBFile *()>(&dummy, "", "SBFile", "SBFile", "()");
+  R.Register<SBFile *(int, const char *, bool)>(&dummy, "", "SBFile", "SBFile",
----------------
I don't think these are right because there nothing here to connect the dummy implementation (used for replay) with the invocation of the actual constructor (happening when recording). (The strings are only used for debugging purposes).

This should be something like: `R.Register<SBFile *()>(&construct<SBFile()>::doit, &dummy, ...)`. Note that the first argument is the same blurb as the thing used in the LLDB_REGISTER_CONSTRUCTOR macro in the constructor, and it's how the reproducer connects the two methods. Maybe after fixing these (you'll need the register FileSP version of the constructor too), you won't need the other changes?

That said, I am surprised you were able to get even this far with this code. Jonas, shouldn't there be some kind of an assertion if you call an unregistered method/constructor during recording?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68434