<div dir="ltr"><div>I've always wished clang-format would make these into tables to reduce the length, but oh well. :(<br></div><div><br></div><div>+inline CXXCtorType toCXXCtorType(StructorType T) {</div><div>+ switch (T) {</div>
<div>+ case StructorType::Complete:</div><div>+ return Ctor_Complete;</div><div>+ case StructorType::Base:</div><div>+ return Ctor_Base;</div><div>+ case StructorType::Deleting:</div><div>+ llvm_unreachable("foo");</div>
<div><br></div><div>The message should have some meaning, i.e. cannot have deleting ctor.</div><div><br></div><div>+ }</div><div>+}</div><div>+</div><div>+inline StructorType getFromCtorType(CXXCtorType T) {</div><div>+ switch (T) {</div>
<div>+ case Ctor_Complete:</div><div>+ return StructorType::Complete;</div><div>+ case Ctor_Base:</div><div>+ return StructorType::Base;</div><div>+ case Ctor_CompleteAllocating:</div><div>+ llvm_unreachable("foo");</div>
<div><br></div><div>I didn't realize we even had this enum. Anyway, "invalid enum" seems like a better message.</div><div><br></div><div>+ }</div><div>+}</div><div>+</div><div>+inline CXXDtorType toCXXDtorType(StructorType T) {</div>
<div>+ switch (T) {</div><div>+ case StructorType::Complete:</div><div>+ return Dtor_Complete;</div><div>+ case StructorType::Base:</div><div>+ return Dtor_Base;</div><div>+ case StructorType::Deleting:</div><div>
+ return Dtor_Deleting;</div><div>+ }</div><div>+}</div><div>+</div><div>+inline StructorType getFromDtorType(CXXDtorType T) {</div><div>+ switch (T) {</div><div>+ case Dtor_Deleting:</div><div>+ return StructorType::Deleting;</div>
<div>+ case Dtor_Complete:</div><div>+ return StructorType::Complete;</div><div>+ case Dtor_Base:</div><div>+ return StructorType::Base;</div><div>+ }</div><div>+}</div><div><br></div><div>If you want to satisfy MSVC I believe you have to put llvm_unreachable at the end of all of these functions. I don't think it does switch case coverage analysis.</div>
<div><br></div><div>Otherwise, the refactoring side looks good.</div><div><br></div><div>-----</div><div><br></div><div><br></div><div><div>+enum CXXCtorComdatType { Ctor_Comdat = Ctor_CompleteAllocating + 1 };</div><div>
+</div><div> /// \brief C++ destructor types.</div><div> enum CXXDtorType {</div><div> Dtor_Deleting, ///< Deleting dtor</div><div>@@ -34,6 +36,8 @@ enum CXXDtorType {</div><div> Dtor_Base ///< Base object dtor</div>
<div> };</div><div> </div><div>+enum CXXDtorComdatType { Dtor_Comdat = Dtor_Base + 1 };</div></div><div><br></div><div>Any strong reason to keep these out of the CXXCtorType and CXXDtorType enums? We have very few switches over them. Especially in the ctor case, where we only implement Ctor_Base and Ctor_Complete.</div>
<div><br></div><div><div>+ virtual void mangleCXXCtorComdat(const CXXConstructorDecl *D, raw_ostream &);</div><div>+ virtual void mangleCXXDtorComdat(const CXXDestructorDecl *D, raw_ostream &);</div></div><div><br>
</div><div>These methods are Itanium-specific, so they can live on ItaniumMangleContext in the same header. We get a little more type safety that way, since the caller has to explicitly downcast to ItaniumMangleContext if they want to call this method.</div>
<div><br></div><div><div>+ // The complete constructor is equivalent to the base constructor</div><div>+ // for classes with no virtual bases.</div><div>+ if (MD->getParent()->getNumVBases())</div><div>+ return StructorCodegen::Emit;</div>
</div><div><br></div><div>Maybe flip the logic of the comment to match the code, like "The complete and base structors are not equivalent if there are any virtual bases, so emit separate functions."?</div><div><br>
</div><div>The logic of emitCXXStructor and getCodegenToUse is starting to feel very Itanium-specific, because it has this requirement that if you must emit the structor in one TU, you must emit all structors. The MS ABI doesn't have this constraint. First, there are no ctor variants, and for dtors, anyone using a complete dtor or deleting dtor is required to emit it like an inline function if they call it. Speaking of which, we never call deleting dtors, so far as I can tell. So, I wonder if we should push emitCXXStructor through the CGCXXABI interface.</div>
<div><br></div><div><div>+ if (CompleteCG == StructorCodegen::COMDAT) {</div><div>+ SmallString<256> Buffer;</div><div>+ llvm::raw_svector_ostream Out(Buffer);</div><div>+ if (DD)</div><div>+ ABI->getMangleContext().mangleCXXDtorComdat(DD, Out);</div>
<div>+ else</div><div>+ ABI->getMangleContext().mangleCXXCtorComdat(CD, Out);</div><div>+ llvm::Comdat *C = TheModule.getOrInsertComdat(Out.str());</div><div>+ Fn->setComdat(C);</div><div>+ }</div></div>
<div><br></div><div>If you push emitCXXStructor into CGCXXABI, the above code will live in ItaniumCXXABI.cpp, and you can access the Itanium-specific mangleCXXCtorComdat mangler methods.</div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Thu, Sep 4, 2014 at 2:19 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">On 4 September 2014 14:41, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>
> I don't like calling arrangeCXXDtorDeclaration from ctor-related functions.<br>
> It looks like a typo. In the past I've used 'structor' as the generic term<br>
> for ctors and dtors. You can see it in the Itanium mangling code, but I<br>
> don't think it's very widely used. Does that seem like a better<br>
> nomenclature? Then we would probably have StructorType, etc.<br>
<br>
</div>The attached patches uses the Structor nomenclature.<br>
<div class=""><br>
> I'm excited to finally have this functionality, though. :)<br>
<br>
</div>+1 :-)<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div>