[PATCH] D21361: [pdbdump] Verify LF_{CLASS, ENUM, INTERFACE, STRUCTURE, UNION} records.

[Zachary Turner via llvm-commits]

zturner added inline comments.

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:78
@@ +77,3 @@
+  if (Obj.getError())
+    return make_error<RawError>(raw_error_code::corrupt_file,
+                                "Corrupt TPI hash table.");
----------------
I'm not sure if `corrupt_file` is the best error code to return here, because you could construct one of these out of thin air instead of coming from a file.  Maybe we should have a new code called `invalid_type_record`.

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:81
@@ +80,3 @@
+
+  auto Opts = static_cast<uint16_t>(Obj->getOptions());
+  if (Opts & static_cast<uint16_t>(codeview::ClassOptions::ForwardReference)) {
----------------
Is the `static_cast` necessary?  The enum should have overloaded `&` operator so you can do the operation directly.

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:81
@@ +80,3 @@
+
+  auto Opts = static_cast<uint16_t>(Obj->getOptions());
+  if (Opts & static_cast<uint16_t>(codeview::ClassOptions::ForwardReference)) {
----------------
zturner wrote:
> Is the `static_cast` necessary?  The enum should have overloaded `&` operator so you can do the operation directly.
What if `T` doesn't have a `getOptions` method?  Will this be used for other values of `T` in the future?  If so I think we should use the visitor instead, since it isolates the handling of each type in a separate function

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:82
@@ +81,3 @@
+  auto Opts = static_cast<uint16_t>(Obj->getOptions());
+  if (Opts & static_cast<uint16_t>(codeview::ClassOptions::ForwardReference)) {
+    // We don't know how to calculate a hash value for this yet.
----------------
Are these two `static_cast`s necessary?  The enum should have overloaded `&` operator so you can do the operation directly.

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:90
@@ +89,3 @@
+  if (!(Opts & static_cast<uint16_t>(codeview::ClassOptions::Scoped))) {
+    Hash = hashStringV1(Obj->getName());
+    return Error::success();
----------------
Is it always `hashStringV1`?  I would think it depends on the version of the `Tpi` stream.  I don't think we have a way to test this as I don't know how to generate a PDB file with a newer tpi stream version.  Does the reference implementation hardcode a call to this function or does it go through a function pointer which might be `hashStringV1` or `hashStringV2`?

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:118
@@ -85,7 +117,3 @@
     break;
-  case LF_ENUM: {
-    ErrorOr<EnumRecord> Enum = EnumRecord::deserialize(TypeRecordKind::Enum, D);
-    if (Enum.getError())
-      return make_error<RawError>(raw_error_code::corrupt_file,
-                                  "Corrupt TPI hash table.");
-    Hash = hashStringV1(Enum->getName());
+  case LF_CLASS:
+    if (auto EC = getTpiHash<ClassRecord, TypeRecordKind::Class>(Rec, Hash))
----------------
If there were less code required to use the visitor, would that be preferable?  I have an idea in mind to drastically reduce the amount of code needed to use it which I've been putting off.  If you think we're going to add more values to this switch statement in subsequent patches, it's probably worth improving the visitor, but if you think this will be all, then maybe it's not necessary.


http://reviews.llvm.org/D21361