JDevlieghere added inline comments.
================
Comment at: source/API/SBReproducerPrivate.h:82-93
+/// Base class for tag dispatch used in the SBDeserializer. Different tags are
+/// instantiated with different values.
+template <unsigned> struct SBTag {};
+
+/// We need to differentiate between pointers to fundamental and
+/// non-fundamental types. See the corresponding SBDeserializer::Read method
+/// for the reason why.
----------------
labath wrote:
> Just curious: What's the advantage of this over just declaring a bunch of Tag
> classes directly (struct PointerTag{}; struct ReferenceTag{}; ...)?
Mostly preference, but the structs are less code so I've simplified it.
================
Comment at: source/API/SBReproducerPrivate.h:121
+public:
+ SBDeserializer(llvm::StringRef buffer = {}) : m_buffer(buffer), m_offset(0)
{}
+
----------------
labath wrote:
> Instead of the m_offset variable, what do you think about just
> `drop_front`ing the appropriate amount of bytes when you are done with the?
> It doesn't look like you'll ever need to go back to an earlier point in the
> stream...
We don't go back, but the buffer is the backing memory for deserialized
c-strings.
================
Comment at: source/API/SBReproducerPrivate.h:182
+ typedef typename std::remove_reference<T>::type UnderlyingT;
+ // If this is a reference to a fundamental type we just read its value.
+ return *m_index_to_object.template GetObjectForIndex<UnderlyingT>(
----------------
labath wrote:
> Is this correct? I would expect the fundamental references to not go through
> this overload at all...
Pretty sure, but please elaborate on why you think that.
================
Comment at: source/API/SBReproducerPrivate.h:453
+public:
+ SBSerializer(llvm::raw_ostream &stream = llvm::outs()) : m_stream(stream) {}
+
----------------
labath wrote:
> Is this default value used for anything? It don't see it being used, and it
> doesn't seem particularly useful anyway, as you'd just get a lot of binary
> junk on stdout.
I'm 100% sure it's needed because I got rid of it only to find out we couldn't
do without it. I'm not entirely sure anymore, but I think we didn't know if the
function was going to have a (useful) return value or not.
================
Comment at: tools/driver/Driver.cpp:913-917
+ SBReproducer reproducer;
+ if (reproducer.Replay()) {
+ return 0;
+ }
+
----------------
labath wrote:
> The driver should know whether it is replaying things or not, right? Can we
> have it call `Replay` only if replay mode has actually been requested?
Replay will check if we're in replay mode, which I think is a more canonical
way to check it. Happy to change this if you disagree.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56322/new/
https://reviews.llvm.org/D56322
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
