[PATCH] D19899: [codeview] Add a type visitor to help abstract away type stream handling

[Reid Kleckner via llvm-commits]

rnk marked 4 inline comments as done.
rnk added a comment.

Thanks! New patch coming


================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:30-36
@@ +29,9 @@
+  bool consumeObject(ArrayRef<uint8_t> &Data, const T *&Res) {
+    if (Data.size() < sizeof(*Res)) {
+      HadError = true;
+      return false;
+    }
+    Res = reinterpret_cast<const T *>(Data.data());
+    Data = Data.drop_front(sizeof(*Res));
+    return true;
+  }
----------------
zturner wrote:
> In the future we could get rid of this entirely and use the `StreamReader` interface in `DebugInfoPDB`.  It wouldn't work right now and some refactoring would have to happen, but I think the design of it is flexible enough to support this in theory.
I agree, we need something that lets us read records from a possibly non-contiguous stream like those in PDBs.

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:66
@@ +65,3 @@
+        break;
+      default:
+        static_cast<Derived *>(this)->visitUnknownType(I.Leaf);
----------------
amccarth wrote:
> I'd put the default either first or last.
done, I didn't like it after the macro goo

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:78
@@ +77,3 @@
+#include "TypeRecords.def"
+      }
+      static_cast<Derived *>(this)->visitTypeEnd(I.Leaf, RecordData);
----------------
amccarth wrote:
>     #undef TYPE_RECORD
> 
> Oh, but that might mess up the other visitor loops below.  Yuck.  This macro looks tightly scoped, but it really isn't.
It is tightly scoped, it's undef'd by the .def file. It's confusing, but it is the standard pattern you see in places like include/llvm/IR/Metadata.def.

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:83
@@ +82,3 @@
+      // bytes here. All tested versions of MSVC include LF_PAD bytes in
+      // LeafData, though. If we find them, call skipPadding.
+    }
----------------
amccarth wrote:
> Is there any harm in just calling skipPadding here?  We do that already for field lists.
> 
> Also, this comment is copied from the iterator.  It seems like it should be in only one place, and I think it makes most sense in the iterator code.
Oops, yeah that's the responsibility of the iterator.

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:107
@@ +106,3 @@
+  /// Action to take on field lists. By default, we visit the members one by
+  /// one.
+  void visitFieldList(TypeLeafKind Leaf, ArrayRef<uint8_t> FieldData) {
----------------
amccarth wrote:
> I'm not clear what you mean by "By default."  This looks like it always visits the members one by one.  Do you mean that the derived class may override this method to get other behavior?
That was the idea, but I don't think there's a real use case for it. I should rewrite this comment to remove that implication.

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:129
@@ +128,3 @@
+  }
+#include "TypeRecords.def"
+      }
----------------
amccarth wrote:
>     #undef MEMBER_RECORD
not needed

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecords.def:20-22
@@ +19,5 @@
+
+#ifndef TYPE_RECORD_ALIAS
+#define TYPE_RECORD_ALIAS(ClassName, LeafEnum) TYPE_RECORD(ClassName, LeafEnum)
+#endif
+
----------------
zturner wrote:
> Why do we need these `_ALIAS` macros?  They seem to do nothing, why not just use `TYPE_RECORD()` everywhere?
I do this with them when stamping out declarations of the visit##ClassType methods:
  #define TYPE_RECORD_ALIAS(ClassName, LeafEnum)

Otherwise we end up re-declaring things like `void visitBaseClass(... const BaseClass *Rec, ...)` multiple times.

The alternative to this might be variadic macros, which would let us emit cases like this, but I felt like that was too much metaprogramming:
  case LF_BCLASS:
  case LF_IBCLASS: { ... }

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:1284
@@ -1249,1 +1283,3 @@
 
+static std::error_code decodeNumerictLeaf(StringRef &Data, APSInt &Num) {
+  ArrayRef<uint8_t> Bytes(reinterpret_cast<const uint8_t *>(Data.data()),
----------------
amccarth wrote:
> Typo:  remove the `t` in the midst of `decodeNumerictLeaf`.
That's a pre-existing issue. I thought maybe David Majnemer did it intentionally, but I'm not sure. I'll ask him and fix this separately, either way.

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:1999
@@ +1998,3 @@
+    Data = Data.drop_front(4);
+  }
+
----------------
amccarth wrote:
> It makes me sad that this formatting detail is back out at this level.
Well, the magic isn't part of the stream in the PDB, so we were going to need to do this eventually.

I should use the consumeUInt32 helper though...


http://reviews.llvm.org/D19899