> I think all the test/CodeGen* changes are good, and should probably land
now.
OK, I'll split off and commit those. It should make this patch easier to
manage :)
================
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}}
};
----------------
Richard Smith wrote:
> 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?
> Can you remind me what the issue is here? This has to do with differing
> operator delete lookup behavior?
Yes, we diagnose the multiple 'operator delete' when 'virtual ~C()' is declared
below, whereas in Itanium we diagnose it when that dtor is defined further on.
================
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}}
};
----------------
Hans Wennborg wrote:
> Richard Smith wrote:
> > 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?
> > Can you remind me what the issue is here? This has to do with differing
> > operator delete lookup behavior?
>
> Yes, we diagnose the multiple 'operator delete' when 'virtual ~C()' is
> declared below, whereas in Itanium we diagnose it when that dtor is defined
> further on.
> Please conditionalize this extra diagnostic on use of the MS ABI.
Done.
>Do we have a predefined macro indicating the ABI? If not, maybe we should add
>one?
No, we don't have a macro for this. We could create one, but it's also easy to
run the test in both modes and set a macro ourselves, I've done that in a few
other tests.
================
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}}
----------------
Richard Smith wrote:
> 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?)
IIRC, Reid told me that John had told him to do the access check in the callee
too when he implemented this.
We still have the access check in the caller.
================
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'}}
----------------
Richard Smith wrote:
> This looks incorrect. We shouldn't be trying to mark the `operator delete` as
> used if the destructor is deleted.
But neither ~D3() nor ~D4() below are 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
----------------
Richard Smith wrote:
> 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.
Hmm, it seems to be controlled by -cxx-abi by us :) I agree this is weird,
though.
Would you be ok a FIXME here and filing a bug with Warren on the cc list? Or
running the test with a fixed target?
================
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
----------------
Richard Smith wrote:
> Please test that this produces diagnostics with the MS ABI, rather than not
> testing it at all.
Done.
================
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() {}
----------------
Richard Smith wrote:
> 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.
But we are. In MS ABI we emit a note on the declaration of '~A()'. On itanium
the note comes later, on the definition of 'class B'.
================
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
----------------
Richard Smith wrote:
> 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?)
In MS ABI we don't require the destructor for B here, so we expect no
diagnostics. I've fixed the test to check that.
================
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
----------------
Richard Smith wrote:
> Why does this test need to force the Itanium ABI?
We fail during codegen with "error: cannot mangle RTTI descriptors for type 'A'
yet", and we need RTTI for the typeid which is used in the test.
This isn't a problem for the '-verify' line though. I'll update that.
================
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
----------------
Richard Smith wrote:
> Why does this test need to force the Itanium ABI?
In MS ABI we don't emit any diagnostics in this test. We should probably take a
second look and verify that's correct behaviour.
I figured the test mostly seemed to be a regression test for the ICE in PR7800,
so maybe we shouldn't run it in MS mode since none of the expectations apply.
================
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
----------------
Richard Smith wrote:
> 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`)?
Done.
================
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
}
----------------
Richard Smith wrote:
> Maybe instead change this test to pass the arguments by reference instead of
> by value?
Done.
http://llvm-reviews.chandlerc.com/D2401
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
