[PATCH] D125721: [CodeView] Combine variable def ranges that are continuous.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Nice!



================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h:76
+        LocalVarDefRange Range;
+        std::memcpy(&Range, &Empty, 8);
+        return Range;
----------------
To minimize the amount of code that needs to know or assume the size of LocalVarDefRange, I recommend exposing a method, maybe called `toOpaqueValue`, which returns a uint64_t. Then, all of these methods don't need to do memcpy, they just call that method, and compare or operate on integer values as you have them. You may also need `createFromOpaqueValue` or something like that.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h:89
+        uint64_t Val;
+        std::memcpy(&Val, &DR, 8);
+        return Val * 37UL;
----------------
Prefer `sizeof(Val)` to hardcoding 8. It's worth adding `static_assert(sizeof(uint64_t) == sizeof(LocalVarDefRange));` to ensure that the bitfields are packed as expected.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h:518
+struct DenseMapInfo<CodeViewDebug::LocalVarDefRange>
+    : CodeViewDebug::LocalVarDefRange::DenseMapInfo {};
+
----------------
Instead of inheritance, can all those methods be defined here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125721