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

Adrian McCarthy via llvm-commits llvm-commits at lists.llvm.org
Wed May 4 08:57:23 PDT 2016


amccarth added inline comments.

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:39
@@ +38,3 @@
+
+  /// Actions to take on known types. By default, does nothing. Regular type
+  /// records describe their own lengths, and take LeafData as is.
----------------
s/does nothing/they do nothing/

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:40
@@ +39,3 @@
+  /// Actions to take on known types. By default, does nothing. Regular type
+  /// records describe their own lengths, and take LeafData as is.
+  /// FIXME: Avoid making FieldData in-out to make the visitor easier to use.
----------------
delete comma

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:58
@@ +57,3 @@
+      ArrayRef<uint8_t> RecordData = LeafData;
+      static_cast<Derived *>(this)->visitTypeBegin(I.Leaf, RecordData);
+      switch (I.Leaf) {
----------------
I would avoid these repeated casts by doing it once and saving the result:

    auto DerivedThis = static_cast<Derived *>(this);


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

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:70
@@ +69,3 @@
+#define TYPE_RECORD(ClassName, LeafEnum)                                       \
+  case LeafEnum: {                                                             \
+    const ClassName *Rec;                                                      \
----------------
I realize the mechanical style rules call for this level of indentation, but it seems the intent would be clearer if this were indented to match the other cases.

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:78
@@ +77,3 @@
+#include "TypeRecords.def"
+      }
+      static_cast<Derived *>(this)->visitTypeEnd(I.Leaf, RecordData);
----------------
    #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.

================
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.
+    }
----------------
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.

================
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) {
----------------
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?

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

================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:137
@@ +136,3 @@
+
+  /// Action to take on unknown members. By default, they are ignored.
+  void visitUnknownMember(TypeLeafKind Leaf) {}
----------------
For the benefit of people making a derived class, I'd make this comment emphasize that processing will stop after the first unknown member.  "Action to take on unknown members" suggests that this will be called for each unknown member.

================
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()),
----------------
Typo:  remove the `t` in the midst of `decodeNumerictLeaf`.

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:1999
@@ +1998,3 @@
+    Data = Data.drop_front(4);
+  }
+
----------------
It makes me sad that this formatting detail is back out at this level.


http://reviews.llvm.org/D19899





More information about the llvm-commits mailing list