[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

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