[PATCH] D158675: [NFC][AsmPrinter] Refactor DbgVariable as a std::variant

Fri Aug 25 12:47:16 PDT 2023

scott.linder added a comment.

In D158675#4617747 <https://reviews.llvm.org/D158675#4617747>, @fdeazeve wrote:

> Everything looks good to me, this is much cleaner! On my first encounter with this class, it took me a long time to decipher what was going on, and the variant design is way more expressive.

I know exactly what you mean! I originally started the change as just a way to prove my own understanding to myself. Without having done it I would never have understood the code to begin with.

I'm glad to hear that was a more universal reaction. I think the class slowly accreted many roles/paths, and was sort of "boiled alive". The fact that the only reasonable remedy before `std::variant` was inheritance or a lot of strife related to using a `union` member for non-POD types probably didn't help either.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h:149
+struct MMILoc {
+  mutable SmallVector<FrameIndexExpr, 1> FrameIndexExprs;
+};
----------------
fdeazeve wrote:
> Not for this patch, but it really bothers me that this is mutable. The original rationale must have been to allow sorting when we call the "get" method, but still...
It is almost spooky how you've pointed out the same things I toyed with while working on the patches originally!

I had a patch in the middle somewhere which just exposed the need to sort in the interface, but scrapped it early on. I think your `set` idea makes a lot of sense and I'll follow up with a patch for that as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158675