labath added a comment.
I haven't looked at the code in details, but some of my high-level concerns are:
- the `LLDB_RECORD_***` macros are getting rather long and repetitive. We
should try to find a way to move some of that stuff into a regular function and
keep the macros for the stuff that cannot be done elsewhere. I'm thinking that
instead of:
lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION, \
stringify_args(__VA_ARGS__));
\
if (lldb_private::repro::InstrumentationData _data =
\
LLDB_GET_INSTRUMENTATION_DATA()) {
\
if (lldb_private::repro::Serializer *_serializer =
\
_data.GetSerializer()) {
\
_recorder.Record(*_serializer, _data.GetRegistry(),
\
&lldb_private::repro::construct<Class Signature>::doit,
\
__VA_ARGS__);
\
_recorder.RecordResult(this, false);
\
} else if (lldb_private::repro::Deserializer *_deserializer =
\
_data.GetDeserializer()) {
\
if (_recorder.ShouldCapture()) {
\
lldb_private::repro::replay_construct<Class Signature>::doit(
\
_recorder, *_deserializer, _data.GetRegistry(),
\
LLVM_PRETTY_FUNCTION);
\
}
\
}
\
}
we could have something like:
lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION, \
stringify_args(__VA_ARGS__));
\
if (lldb_private::repro::InstrumentationData _data =
LLDB_GET_INSTRUMENTATION_DATA() /* is this needed? could the Recorder object
tell us when replay is enabled? */) {
lldb_private::repro::Record/*?*/<lldb_private::repro::construct<Class
Signature>>(_recorder, _data/*?*/, anything_else_you_need, __VA_ARGS__)
}
and have the `Record` ( `HandleAPI` ?) function handle the details of what
should happen in different reproducer modes. The above assumes that
`lldb_private::repro::construct` becomes a class which has not one but two
member functions -- basically the merge of the `construct` and
`replay_construct` functions from the current patch, so that the `Record`
function can select the right operation based on the current mode.
- the chopping of LLVM_PRETTY_FUNCTION in `Registry::CheckSignature` sounds
sub-optimal. It feels like we should be able to obtain the "ids" of both
"expected" and "actual" methods and compare those (and have the PRETTY_FUNCTION
just be there for error reporting purposes).
- this functionality should have a unit test just like the existing
ReproducerInstrumentation.cpp functionality. At that point it should be
possible/reasonable to move this patch to the front of the patch set and have
the SB wiring come after the underlying infrastructure is set up.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77602/new/
https://reviews.llvm.org/D77602
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
