[PATCH] D65429: Improving CodeView debug info type record's inline comments

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp:30
+
+static const EnumEntry<uint16_t> ClassOptionNames[] = {
+    ENUM_ENTRY(ClassOptions, Packed),
----------------
Please move these tables to EnumTables.cpp, add `getFooNames` methods for them to EnumTables.h, and use them here. Otherwise these tables will be duplicated in the final binary.


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp:210
+
+static StringRef getMemberAttributes(MemberAccess Access, MethodKind Kind,
+                                     MethodOptions Options) {
----------------
I think most members are very simple, and it would be better to keep the comment on a single line if possible. Something like:
.long ...   # Attrs: Public, Virtual, Sealed
.long ...   # Attrs: Private, Vanilla
.long ...   # Attrs: Public, Vanilla, CompilerGenerated

It's not as explicit as the old comment format, but I think it's better to be more compact.


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp:445
+
+  SmallString<512> Attr("Attributes\n");
+  SmallString<256> PtrType = getEnumName(unsigned(Record.getPointerKind()),
----------------
Please avoid computing these expensive strings when type record mapping is not being used in streaming mode. Formatting strings can be quite expensive, but serializing the records should be as fast as possible. When we aren't streaming, the comment can be an empty string or just "Attributes" or "Attrs".


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp:461
+
+  Attr += ("[\nPtrType: " + PtrType + "\nPtrMode: " + PtrMode + "\nIsFlat: " +
+           itostr(PtrIsFlat) + "\nIsConst: " + itostr(PtrIsConst) +
----------------
I'm also a bit concerned that the vast majority of pointers don't set any of these flags. It would be nice to only mention the flags that are set. You can also look at the code in llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp, which emits flags like that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65429