[PATCH] D60018: [codeview] Remove Type member from CVRecord

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

I like when removing code makes a class actually less complex :)



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:33
 
-  bool valid() const { return Type != static_cast<Kind>(0); }
+  bool valid() const { return kind() != static_cast<Kind>(0); }
 
----------------
`return RecordData.size() >= sizeof(RecordPrefix)`

Do we expect an invalid (zero) `.RecordKind` here?


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:39
+    if (RecordData.size() < sizeof(RecordPrefix))
+      return Kind(0);
+    return static_cast<Kind>(static_cast<uint16_t>(
----------------
`return {};`


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h:54
                                  CodeViewContainer Container) {
-    CVSymbol Result;
-    Result.Type = static_cast<SymbolKind>(Sym.Kind);
+    uint8_t PreBytes[sizeof(RecordPrefix)];
+    RecordPrefix *Prefix = reinterpret_cast<RecordPrefix *>(&PreBytes[0]);
----------------
This seems like a recurrent pattern, why not add a constructor such as:
`CVRecord(RecordPrefix* R) : RecordData((uint8_t*)R, R->RecordLen + 2) {}`

Which reduces all this code to:
```
RecordPrefix R{ulittle16_t(2), ulittle16_t((uint16_t)Sym.Kind)};
CVSymbol Result(&R);
```
Which could be further improved if RecordPrefix had a `RecordPrefix(uint16_t, uint16_t)` constructor.



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h:56
+    RecordPrefix *Prefix = reinterpret_cast<RecordPrefix *>(&PreBytes[0]);
+    Prefix->RecordLen = 0;
+    Prefix->RecordKind = uint16_t(Sym.Kind);
----------------
Should be 2.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h:123
+    RecordPrefix Pre;
+    CVType FieldList = makeFakeFieldList(Pre);
     consumeError(Mapping.Mapping.visitTypeBegin(FieldList));
----------------
Remove `makeFakeFieldList` and make this:
```
RecordPrefix R{ulittle16_t(2), ulittle16_t((uint16_t)TypeLeafKind::LF_FIELDLIST)};
CVSymbol Result(&R);
```


================
Comment at: llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp:70
+  // Seed the first trecord with an appropriate record prefix.
+  uint8_t PreBytes[sizeof(RecordPrefix)];
+  RecordPrefix *Prefix = reinterpret_cast<RecordPrefix *>(&PreBytes[0]);
----------------
See comment above.


================
Comment at: llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp:180
 std::vector<CVType> ContinuationRecordBuilder::end(TypeIndex Index) {
-  CVType Type;
-  Type.Type = getTypeLeafKind(*Kind);
+  uint8_t PreBytes[sizeof(RecordPrefix)];
+  RecordPrefix *Prefix = reinterpret_cast<RecordPrefix *>(&PreBytes[0]);
----------------
Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018