================
Comment at: test/CodeGenCXX/microsoft-abi-thunks.cpp:44
@@ +43,3 @@
+ protected:
+ virtual void protected_f();
+ // MANGLING-DAG: @"\01?protected_f@C@@O3AEXXZ"
----------------
Reid Kleckner wrote:
> Why no thunks for the protected one?
O3/O7 **is** the thunk mangling.
I've added the non-thunk version mangling check just to avoid confusion.
================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:189
@@ +188,3 @@
+ void setThunkLinkage(llvm::Function *Thunk, bool AvailableExternally) {
+ assert(AvailableExternally && "All thunks should be available externally");
+ Thunk->setLinkage(llvm::GlobalValue::WeakAnyLinkage);
----------------
Reid Kleckner wrote:
> Why? The MS C++ ABI has no key functions, so nothing is guaranteed to be
> available externally, basically ever.
Hm, it looks like I've just put a redundant assert at the wrong place so it
stopped making any sense.
If AvailableExternally==false, EmitThunk() will assert anyways when called from
EmitThunks() on out-of-line method definition - because EmitThunk() was already
called by MaybeEmitThunkAvailableExternally.
I've just removed this assert.
================
Comment at: lib/AST/MicrosoftMangle.cpp:1420
@@ -1418,2 +1419,3 @@
switch (MD->getAccess()) {
default:
+ llvm_unreachable("Unsupported access specifier");
----------------
Reid Kleckner wrote:
> Make this "case AS_none", and rely on -Wcovered-switch to ensure that we
> don't miss any cases.
OK, done
================
Comment at: lib/AST/MicrosoftMangle.cpp:1881
@@ +1880,3 @@
+ switch (MD->getAccess()) {
+ default:
+ llvm_unreachable("Unsupported access specifier");
----------------
Reid Kleckner wrote:
> AS_none
Done
================
Comment at: lib/AST/MicrosoftMangle.cpp:1884
@@ +1883,3 @@
+ case AS_private:
+ Out << "G";
+ break;
----------------
Reid Kleckner wrote:
> Nitty micro-optimization: when you have a character, just stream the
> character. It's hard to optimize the string stream back the character stream.
Good catch - done
================
Comment at: lib/AST/MicrosoftMangle.cpp:1897
@@ +1896,3 @@
+ switch (MD->getAccess()) {
+ default:
+ llvm_unreachable("Unsupported access specifier");
----------------
Reid Kleckner wrote:
> AS_none
Done
================
Comment at: lib/AST/MicrosoftMangle.cpp:1933
@@ +1932,3 @@
+ const ThisAdjustment &Adjustment, raw_ostream &Out) {
+ // FIXME: Actually, the dtor thunk should be emitted for vector deleting
+ // dtors rather than scalar deleting dtors. Just use the vector deleting dtor
----------------
Reid Kleckner wrote:
> Peter Collingbourne published a patch for these many months ago that I think
> works. Feel free to ping it or take it over. I'm OK with this FIXME for now
> though.
That patch had some problems that you've later resolved while working on
regular dtors. It's very likely it still need some work, but probably not too
much.
Let's leave the FIXME here for a while, I'll consider taking over Peter's patch
later.
================
Comment at: lib/AST/MicrosoftMangle.cpp:1942
@@ -1902,1 +1941,3 @@
+ Mangler.mangleFunctionType(DD->getType()->castAs<FunctionProtoType>(),
+ DD, true, true);
}
----------------
Reid Kleckner wrote:
> "true, true" here is gross. The other booleans without names are also gross.
> We should make up and pass in a proper enum along the lines of:
> enum FunctionDeclType {
> FDT_FreeFunctoin
> FDT_InstanceMethod
> FDT_StructorMethod
> };
>
> All structors are instance methods, so we don't need the 4th state.
>
> Feel free to commit that cleanup change separately with post-commit review.
Come on, we can do better than that - see r191950 :)
Thanks for catching!
================
Comment at: lib/CodeGen/CGCXXABI.h:357
@@ -347,1 +356,3 @@
+ bool AvailableExternally) = 0;
+
virtual void EmitReturnFromThunk(CodeGenFunction &CGF,
----------------
Reid Kleckner wrote:
> These two together don't seem like the right interface. I'll have more
> comments later.
After thinking for some time, I've changed the way CGVTables handles thunks and
now the interface is simpler and (likely) more correct.
PTAL!
http://llvm-reviews.chandlerc.com/D1787
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
