[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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits