[PATCH] D19887: Move CodeViewTypeStream and iterators to DebugInfo/CodeView

[David Blaikie via llvm-commits]

dblaikie added a subscriber: dblaikie.

================
Comment at: include/llvm/DebugInfo/CodeView/TypeStream.h:34
@@ +33,3 @@
+
+static std::error_code consumeUInt32(StringRef &Data, uint32_t &Res) {
+  const ulittle32_t *IntPtr;
----------------
static->inline as this code is in a header (otherwise you get ODR violations, code duplication, etc) - same goes for any other non-member static things here (consumeObject above)

================
Comment at: include/llvm/DebugInfo/CodeView/TypeStream.h:123-129
@@ +122,9 @@
+
+bool operator==(const TypeIterator &lhs, const TypeIterator &rhs) {
+  return (lhs.Data.begin() == rhs.Data.begin()) || (lhs.AtEnd && rhs.AtEnd);
+}
+
+bool operator!=(const TypeIterator &lhs, const TypeIterator &rhs) {
+  return !(lhs == rhs);
+}
+
----------------
'inline' now that these are in a header, otherwise you'll get duplicate symbols when two object files using that include this header are linked together.

Moving these definitions into their friend declarations would make them implicitly inline and they're quite small anyway, so that's probably not a bad idea.

================
Comment at: include/llvm/DebugInfo/CodeView/TypeStream.h:131
@@ +130,3 @@
+
+class TypeStream {
+public:
----------------
I'm not sure about this type, with regard to our previous discussions on "where to put the magic" and how to fail the ctor.

If this type is just for range-for convenience, should probably just use llvm::iterator_range to put the two TypeIterators together.

But if we want to use it to make the magic access a bit more natural/remove the extra memory footprint from the iterators, then I'd expect to see the knowledge of magic removed from the iterator and more of the initialization work to be done in TypeStream's ctor instead, which seems OK. Then maybe the iterators would be a private implementation detail of TypeStream, or at least have some private API that (via friendship) only TypeStream can use (for construction, etc).

(still missing the error handling here - I'd imagine TypeStream would have a named ctor that returns ErrorOr<TypeStream> - not sure about errors during iteration (beyond the initial construction). That gets trickier)

================
Comment at: include/llvm/DebugInfo/CodeView/TypeStream.h:133-136
@@ +132,6 @@
+public:
+  TypeStream(const StringRef &Data) {
+    Begin = TypeIterator(Data);
+    End = TypeIterator();
+    Magic = Begin.getMagic();
+  }
----------------
Prefer to use the initializer list to initialize members (& you shouldn't need to initialize 'End' at all in that case, since it's default constructed)

================
Comment at: include/llvm/DebugInfo/CodeView/TypeStream.h:141
@@ +140,3 @@
+  TypeIterator end() const { return End; }
+  unsigned magic() const { return Magic; }
+
----------------
Difficult question about style: while begin/end need to be spelled that way, magic here could match the LLVM coding conventions and be called 'getMagic' or similar.


http://reviews.llvm.org/D19887