<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">So back to 0002.<div class=""><br class=""></div><div class="">You were worried about the lack of protection to ensure that a subclass of Constant actually implements destroyConstant. Unfortunately I couldn’t find a good way to check for this statically, but I did go ahead and implement the alternative you suggested. For context, what follows (indented) was the proposal which i’ve basically just implemented exactly as described. It seems to be a very good way to handle this, and isn’t much more code churn.</div><blockquote style="margin: 0 0 0 40px; border: none; padding: 0px;" class=""><div class=""><br class=""></div><div class="">However, I realize there's already something called</div><div class="">`Constant::destroyConstantImpl()`, which has a `delete this` inside it</div><div class="">(ironic, given that the context for this patch is removing the vtable</div><div class="">that the `delete` call relies on).</div><div class=""><br class=""></div><div class="">I haven't looked at all the patches in this series yet, but I feel like</div><div class="">there ought to be some way of clarifying `Constant` destruction</div><div class="">immediately. My shot-from-the-hip is something like the following:</div><div class=""><br class=""></div><div class=""> class Constant {</div><div class=""> public:</div><div class=""> void destroyConstant();</div><div class=""> };</div><div class=""><br class=""></div><div class=""> class SomeConstant : public Constant {</div><div class=""> friend class Base; // For fooImpl().</div><div class=""><br class=""></div><div class=""> /// Destroy and delete the constant.</div><div class=""> void destroyConstantImpl();</div><div class=""> ~SomeConstant();</div><div class=""><br class=""></div><div class=""> // Don't provide destroyConstant().</div><div class=""> };</div><div class=""><br class=""></div><div class=""> void Constant::destroyConstant() {</div><div class=""> // Remove lingering references from the constant pool (move from</div><div class=""> // old `Constant::destroyConstantImpl()`).</div><div class=""> while (!use_empty()) {</div><div class=""> // ...</div><div class=""> }</div><div class=""><br class=""></div><div class=""> // Dispatch to subclass to cleanup and delete.</div><div class=""> switch (...) {</div><div class=""> default:</div><div class=""> llvm_unreachable(...);</div><div class=""> // Compile error if there's an unhandled case instead of</div><div class=""> // infinite recursion.</div><div class=""> #define HANDLE_CONSTANT(NAME) \</div><div class=""> case NAME ## Kind: \</div><div class=""> cast<NAME>(this)->destroyConstantImpl(); \</div><div class=""> break;</div><div class=""> }</div><div class=""><br class=""></div><div class=""> // When we drop virtual dispatch for the destructor, move the</div><div class=""> // delete call inside the switch statement above.</div><div class=""> delete this;</div><div class=""> }</div><div class=""><br class=""></div><div class=""> void SomeConstant::destroyConstantImpl() {</div><div class=""> assert(use_empty() && ...);</div><div class=""> getContext()->SomeConstantPool.erase(this);</div><div class=""> }</div><div class=""><br class=""></div><div class="">This inverts the destroyConstant/Impl relationship.</div><div class=""><br class=""></div><div class="">Maybe this leaves out some case, or doesn't quite fit with the end goal</div><div class="">(you've thought about this more than I have). My main point is, with</div><div class="">static dispatch we can easily catch the "missing destroyConstant()</div><div class="">implementation" at compile-time, and we should.</div></blockquote><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Pete</div></body></html>