[PATCH] D63662: Changing CodeView Debug info Type record output

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CodeViewRecordIO.h:35
+  virtual void EmitBinaryData(StringRef Data) = 0;
+  virtual ~CodeViewRecordStreamer(){};
+};
----------------
extra ';' at the end of this line (& could use "= default;" instead of "{}")


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CodeViewRecordIO.h:192-193
+    if (isStreaming()) {
+      return StreamedLen;
+    } else {
+      return 0;
----------------
LLVM style tends to avoid "else" after a conditional return/break/etc: https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CodeViewRecordIO.h:211-213
+    if (isStreaming()) {
+      StreamedLen = 4; // The record prefix is 4 bytes long
+    }
----------------
LLVM code doesn't usually have braces around single-line blocks (here and elsewhere in this review)


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp:269
   uint16_t Size;
-  if (IO.isWriting()) {
+  if (!IO.isReading()) {
     ArrayRef<VFTableSlotKind> Slots = Record.getSlots();
----------------
What else can you be doing other than reading or writing? (what is the semantic difference in this (& similar) code change?)


================
Comment at: llvm/test/DebugInfo/COFF/asm.ll:2
 ; RUN: llc -mcpu=core2 -mtriple=i686-pc-win32 -O0 < %s | FileCheck --check-prefix=X86 %s
+; RUN: llc -mcpu=core2 -mtriple=i686-pc-win32 -o - -O0 -filetype=obj < %s | llvm-readobj -S --sr --codeview | FileCheck --check-prefix=OBJ32 %s
 ; RUN: llc -mcpu=core2 -mtriple=i686-pc-win32 -o - -O0 < %s | llvm-mc -triple=i686-pc-win32 -filetype=obj | llvm-readobj -S --sr --codeview | FileCheck --check-prefix=OBJ32 %s
----------------
Seems like overkill to double all these tests, perhaps?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63662/new/

https://reviews.llvm.org/D63662