[Lldb-commits] [PATCH] D78141: [lldb/Reproducers] Simplify LLDB_RECORD macros

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This looks even better than I hoped. I think this is a worthwhile simplification even without the followup patches. Just a couple of questions inline...



================
Comment at: lldb/include/lldb/Utility/ReproducerInstrumentation.h:117-135
 #define LLDB_RECORD_CONSTRUCTOR(Class, Signature, ...)                         \
   lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION,                \
                                           stringify_args(__VA_ARGS__));        \
   if (lldb_private::repro::InstrumentationData _data =                         \
           LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
     _recorder.Record(_data.GetSerializer(), _data.GetRegistry(),               \
                      &lldb_private::repro::construct<Class Signature>::doit,   \
----------------
Could you merge these two in a similar way as well (not with the method macros, just with themselves)? I know that the constructors could be implemented as a function in the other patch, but I have a feeling it will be pretty confusing if two very similar functionalities were implemented in completely different ways...


================
Comment at: lldb/include/lldb/Utility/ReproducerInstrumentation.h:137
 
-#define LLDB_RECORD_METHOD(Result, Class, Method, Signature, ...)              \
+#define LLDB_RECORD_(Method, T1, T2, ...)                                      \
   lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION,                \
----------------
idea: Should we standardize the name of the inner class and drop the `Method` argument?


================
Comment at: lldb/include/lldb/Utility/ReproducerInstrumentation.h:757-760
+  void Record(Serializer &serializer, Registry &registry, void (*f)(),
+              const EmptyArg &arg) {
+    Record(serializer, registry, f);
+  }
----------------
Why the void overload? It looks like the templated version would work just fine for void too...


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

https://reviews.llvm.org/D78141