[PATCH] D23177: [CodeView] Decouple the type record deserialization from the visitor.

[Adrian McCarthy via llvm-commits]

amccarth added inline comments.

================
Comment at: include/llvm/DebugInfo/CodeView/RecordSerialization.h:147
@@ -146,3 +146,3 @@
 
   Error deserialize(ArrayRef<uint8_t> &Data) const {
     if (Data.empty())
----------------
Method name seems weird given the struct name.

================
Comment at: include/llvm/DebugInfo/CodeView/RecordSerialization.h:150
@@ -152,1 +149,3 @@
+      return make_error<CodeViewError>(cv_error_code::insufficient_buffer,
+                                       "Null terminated string is empty!");
 
----------------
As long as you're improving the message, how about adding the missing hyphen?  "Null-terminated string..."

================
Comment at: include/llvm/DebugInfo/CodeView/RecordSerialization.h:161
@@ -161,3 +160,3 @@
             cv_error_code::insufficient_buffer,
-            "Null terminated string buffer is empty!");
+            "Null terminated string has no null terminator!");
     }
----------------
Another hyphen needed:  "Null-terminated string has no null terminator!"

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:117
@@ -116,2 +116,3 @@
 public:
+  explicit ModifierRecord(TypeRecordKind Kind) : TypeRecord(Kind) {}
   ModifierRecord(TypeIndex ModifiedType, ModifierOptions Modifiers)
----------------
For many of these, there is only one Kind that should be passed in.  I'm concerned that by adding a new constructor, we make it possible to create a nonsense record with the wrong Kind.  Perhaps an assertion would be appropriate in these cases.

I'm OK if you don't think this is worth it.  Some of the record types do depend on a passed in Kind, and there's no check there, so this isn't an entirely new problem.

================
Comment at: tools/llvm-readobj/llvm-readobj.h:26
@@ -25,2 +25,3 @@
   LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg);
+  void error(llvm::Error ec);
   void error(std::error_code ec);
----------------
majnemer wrote:
> ruiu wrote:
> > EC
> This looks like a redeclaration.
This seems to duplicate the prototype two lines down, except for the case of the parameter name.


https://reviews.llvm.org/D23177