[PATCH] D22884: [codeview] Emit vftable records

[Adrian McCarthy via llvm-commits]

amccarth added inline comments.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2096
@@ +2095,3 @@
+  for (const DIDerivedType *VFT : DeferredVFTables)
+    getMSVFTableTypeIndex(VFT);
+}
----------------
Just a rant about this pattern ... I don't see an easy way to fix this and be consistent with existing code.

The comment says this loop _emits_ stuff, but the loop looks like it just looks up type indices and drops them on the floor.  In reality, we're calling this "get..." function for its side effects.  The function name doesn't even hint at the side effects.  Many people expect methods like "getFoo" to be const accessor methods.  And if they're not const, it's probably because there's a little caching involved.  But the side effects here involve more than caching.

This seems to be a pervasive anti-pattern in the bits of LLVM I've seen, a pattern that makes it hard (for me, at least) to understand the code.  Without that comment, I'd be lost.  With it, I at least know I have to go study the guts of what looks like an accessor method to figure out the side effect.  It's especially confusing, since many methods have verbs like `emit` in them.

Anyway, just a rant.  I don't expect you to make any changes.


https://reviews.llvm.org/D22884