I wonder if we're being too eager when performing `operator delete` lookup
for a class with a virtual destructor. Instead of performing the lookup when
the destructor is declared, could we defer it until the vtable is used?
================
Comment at: test/CXX/class.access/p4.cpp:129
@@ -123,1 +128,3 @@
+#endif
+ void foo(A param) { // Okay in Itanium.
A local; // expected-error {{variable of type 'test3::A' has private
destructor}}
----------------
Why do we do the access check in the callee under the MS ABI? We can still do
dtor access checks in the caller, even though we actually make the call in the
callee, can't we? Presumably MSVC redundantly checks access in both places?
(Do we need to omit the access checking in the caller for compatibility
somehow?)
================
Comment at: test/CXX/drs/dr2xx.cpp:543
@@ -542,3 +542,3 @@
struct C : A, B {
- virtual ~C();
+ virtual ~C(); // expected-error 0-1 {{'operator delete' found in multiple
base classes}}
};
----------------
Reid Kleckner wrote:
> So, I remember we decided to do it this way, but now we've essentially
> weakened this test to pass if we fail to diagnose this in the Itanium C++
> ABI. Can you remind me what the issue is here? This has to do with
> differing operator delete lookup behavior?
Please conditionalize this extra diagnostic on use of the MS ABI. Do we have a
predefined macro indicating the ABI? If not, maybe we should add one?
================
Comment at: test/CXX/special/class.dtor/p5-0x.cpp:98-104
@@ -96,2 +97,9 @@
} d2; // expected-error {{deleted function}}
+#ifdef MSABI
+// expected-error@+7 {{no suitable member 'operator delete' in 'D3'}}
+// expected-note@+7 {{member 'operator delete' declared here}}
+// expected-note@+7 {{member 'operator delete' declared here}}
+// expected-error@+9 {{attempt to use a deleted function}}
+// expected-note@+9 {{function has been explicitly marked deleted here}}
+#endif
struct D3 { // expected-note {{virtual destructor requires an unambiguous,
accessible 'operator delete'}}
----------------
This looks incorrect. We shouldn't be trying to mark the `operator delete` as
used if the destructor is deleted.
================
Comment at: test/Sema/empty1.c:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify -Wc++-compat
+// RUN: %clang_cc1 %s -cxx-abi itanium -fsyntax-only -verify -Wc++-compat
----------------
This looks weird. I think you're adding this because empty structs have size 4
under MSVC, but that's controlled by the psABI, not the C++ ABI.
================
Comment at: test/SemaCXX/destructor.cpp:86-93
@@ -84,6 +85,10 @@
+#ifndef MSABI
+// This bug, "Clang instantiates destructor for function argument" is intended
+// behaviour in the Microsoft ABI because the callee needs to destruct the
arguments.
namespace PR6709 {
template<class T> class X { T v; ~X() { ++*v; } };
void a(X<int> x) {}
}
+#endif
----------------
Please test that this produces diagnostics with the MS ABI, rather than not
testing it at all.
================
Comment at: test/SemaCXX/destructor.cpp:108-117
@@ -102,6 +107,12 @@
+#ifdef MSABI
+ // expected-note@+2 {{in instantiation of member function
'test6::A<int>::operator delete' requested here}}
+#endif
virtual ~A() {}
};
- class B : A<int> { B(); }; // expected-note {{in instantiation of member
function 'test6::A<int>::operator delete' requested here}}
+#ifndef MSABI
+ // expected-note@+2 {{in instantiation of member function
'test6::A<int>::operator delete' requested here}}
+#endif
+ class B : A<int> { B(); };
B::B() {}
----------------
There's something odd here. We should be producing a note indicating the place
at which the instantiation of `A<int>` was triggered under the MS ABI.
================
Comment at: test/SemaCXX/implicit-virtual-member-functions.cpp:7-16
@@ -5,10 +6,12 @@
+#ifndef MSABI
struct B : A { // expected-error {{no suitable member 'operator delete' in
'B'}}
virtual void f();
void operator delete (void *, int); // expected-note {{'operator delete'
declared here}}
};
void B::f() { // expected-note {{implicit destructor for 'B' first required
here}}
}
+#endif
----------------
Can you provide an alternative set of expected diagnostics here for MS ABI
rather than disabling that half of the test? (I assume the point is that we
trigger `operator delete` lookup earlier?)
================
Comment at: test/SemaCXX/undefined-internal.cpp:1-4
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -cxx-abi itanium -fsyntax-only -verify %s
// Make sure we don't produce invalid IR.
-// RUN: %clang_cc1 -emit-llvm-only %s
+// RUN: %clang_cc1 -cxx-abi itanium -emit-llvm-only %s
----------------
Why does this test need to force the Itanium ABI?
================
Comment at: test/SemaCXX/virtual-base-used.cpp:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -cxx-abi itanium -verify %s
// PR7800
----------------
Why does this test need to force the Itanium ABI?
================
Comment at: test/SemaCXX/warn-weak-vtables.cpp:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify -Wweak-vtables
-Wweak-template-vtables
+// RUN: %clang_cc1 %s -fsyntax-only -verify -cxx-abi itanium -Wweak-vtables
-Wweak-template-vtables
----------------
Can you also test this in MS ABI mode, to ensure the warning is suppressed
there? Something like `-Wweak-vtables -Wweak-template-vtables -Werror` (with no
`-verify`)?
================
Comment at: test/SemaTemplate/virtual-member-functions.cpp:89-93
@@ -82,5 +88,7 @@
+#ifndef MSABI
void test_X(X<int> xi, X<float> xf) {
xi.f();
}
+#endif
}
----------------
Maybe instead change this test to pass the arguments by reference instead of by
value?
http://llvm-reviews.chandlerc.com/D2401
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
