r178563 - If a defaulted special member is implicitly deleted, check whether it's

[Nico Weber]

On Thu, Apr 18, 2013 at 4:46 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Thu, Apr 18, 2013 at 1:15 PM, Nico Weber <thakis at chromium.org> wrote:
>
>> On Thu, Apr 18, 2013 at 11:10 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> On Thu, Apr 18, 2013 at 10:50 AM, Nico Weber <thakis at chromium.org>wrote:
>>>
>>>> Filed PR15783, thanks.
>>>>
>>>>  The WebKit code was added here and seems to be doing what the people
>>>> who wrote it want:
>>>> https://bugs.webkit.org/show_bug.cgi?id=105026
>>>> https://bugs.webkit.org/attachment.cgi?id=179485&action=review
>>>>
>>>
>>>  Looks like it's trying to determine whether this will compile:
>>>
>>> template <>
>>> template <typename T>
>>> void
>>> MemoryInstrumentation::InstrumentationSelector<true>::reportObjectMemoryUsage(const
>>> T* object, MemoryObjectInfo* memoryObjectInfo)
>>> {
>>>     object->reportMemoryUsage(memoryObjectInfo);
>>> }
>>>
>>> For that, it should really be doing SFINAE on the
>>> "object->reportMemoryUsage(memoryObjectInfo)" expression.
>>>
>>
>> Is that possible when using only C++03 features? Checking for
>> T::reportMemoryUsage in a parameter list will miss cases where only T's
>> superclass implements reportMemoryUsage.
>>
>
> Should be. Which compilers do you need this to work on? :)
>

Nothing too exotic: gcc4.6+, clang ~trunk-ish, msvs2010+


>
>   On Thu, Apr 18, 2013 at 10:43 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>>>>
>>>>> On Thu, Apr 18, 2013 at 10:09 AM, Nico Weber <thakis at chromium.org>wrote:
>>>>>
>>>>>> Hi Richard,
>>>>>>
>>>>>> this breaks compilation of this bit of code in WebKit, which does
>>>>>> metaprogramming to check if a class implements a given method:
>>>>>>
>>>>>> template <typename Type> class IsInstrumented {
>>>>>>   class yes { char m; };
>>>>>>   class no { yes m[2]; };
>>>>>>   struct BaseMixin {
>>>>>>     void reportMemoryUsage() const {}
>>>>>>   };
>>>>>>   struct Base : public Type, public BaseMixin {
>>>>>>   };
>>>>>>   template <typename T, T t> class Helper {
>>>>>>   };
>>>>>>   template <typename U>
>>>>>>   static no
>>>>>>   deduce(U *, Helper<void(BaseMixin::*)() const,
>>>>>> &U::reportMemoryUsage> * = 0);
>>>>>>   static yes deduce(...);
>>>>>> public:
>>>>>>   static const bool result = sizeof(yes) == sizeof(deduce((Base
>>>>>> *)(0)));
>>>>>> };
>>>>>>
>>>>>> template <typename T> bool hasReportMemoryUsage(const T *object) {
>>>>>>   return IsInstrumented<T>::result;
>>>>>> }
>>>>>>
>>>>>> class Timer {
>>>>>>   void operator delete(void *p) {}
>>>>>> public:
>>>>>>   virtual ~Timer();
>>>>>> };
>>>>>> bool f() {
>>>>>>   Timer m_cacheTimer;
>>>>>>   return hasReportMemoryUsage(&m_cacheTimer);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> The error message looks like this:
>>>>>>
>>>>>> $ clang -c repro.ii -std=gnu++11  # works in older clang
>>>>>> $ third_party/llvm-build/Release+Asserts/bin/clang -c repro.ii
>>>>>> -std=gnu++11
>>>>>> repro.ii:7:10: error: deleted function '~Base' cannot override a
>>>>>> non-deleted function
>>>>>>   struct Base : public Type, public BaseMixin {
>>>>>>          ^
>>>>>> repro.ii:13:51: note: in instantiation of member class
>>>>>> 'IsInstrumented<Timer>::Base' requested here
>>>>>>   deduce(U *, Helper<void(BaseMixin::*)() const,
>>>>>> &U::reportMemoryUsage> * = 0);
>>>>>>                                                   ^
>>>>>> repro.ii:13:3: note: while substituting deduced template arguments
>>>>>> into function template 'deduce' [with U = IsInstrumented<Timer>::Base]
>>>>>>   deduce(U *, Helper<void(BaseMixin::*)() const,
>>>>>> &U::reportMemoryUsage> * = 0);
>>>>>>   ^
>>>>>> repro.ii:20:10: note: in instantiation of template class
>>>>>> 'IsInstrumented<Timer>' requested here
>>>>>>   return IsInstrumented<T>::result;
>>>>>>          ^
>>>>>> repro.ii:30:10: note: in instantiation of function template
>>>>>> specialization 'hasReportMemoryUsage<Timer>' requested here
>>>>>>   return hasReportMemoryUsage(&m_cacheTimer);
>>>>>>          ^
>>>>>> repro.ii:26:11: note: overridden virtual function is here
>>>>>>   virtual ~Timer();
>>>>>>           ^
>>>>>> 1 error generated.
>>>>>>
>>>>>>
>>>>>> Is this expected? I suppose so, but the diagnostic is a bit hard to
>>>>>> read (it's not clear to me why ~Base() is deleted).
>>>>>>
>>>>>
>>>>> Yes, the diagnostic seems to be correct, if unhelpful. Feel free to
>>>>> file a bug on the diagnostic (if not, I'll try to remember to fix this but
>>>>> can't promise!). We should be pointing out that the function is implicitly
>>>>> deleted because it would otherwise try to call an inaccessible
>>>>> class-specific 'operator delete'.
>>>>>
>>>>
> OK, I've dug up the actual code in WebKit, and it looks like there's a
> rejects-valid bug here too. We reject this:
>
> struct A { void operator delete(void*, long); }; struct B : protected A {
> virtual ~B(); }; struct C : B {};
>
> because the operator delete is apparently inaccessible. Even though it's
> not. My (near-trunk) checkout of g++ and EDG 4.6 also reject this, so I'm a
> bit confused about what's going on here.
>
> For a C++98 version of the same behavior, we reject this:
>
> struct A { void operator delete(void*, long); }; struct B : protected A {
> virtual ~B(); }; struct C : B { ~C() { operator delete(0, 0); } };
>
> Here, we are fine with the explicit call to 'operator delete' in the
> destructor, but the implicit call generates an error "no suitable member
> 'operator delete' in 'C'", due to an access failure.
>
> The WebKit bug was fixed here (to the point where Clang should accept it):
>
>
> http://trac.webkit.org/changeset/147751/trunk/Source/WebCore/platform/Timer.h
>

Yes, but it seems a bit unfortunate that the SFINA implementation detail
prevents private inheritance many files away.


>
>
>>   It compiles fine if I give Base an explicit destructor. Is this the
>>>>>> right fix?
>>>>>>
>>>>>
>>>>> It's inelegant, but yes, that should work (unless you use this with a
>>>>> final class or similar...).
>>>>>
>>>>
> I take that back, the explicit destructor just transforms the example from
> ill-formed to ill-formed (no diagnostic required), because your destructor
> is odr-used (because it's virtual) but not defined (because it can't be).
> But fixing our access check should solve that.
>

Ok, then I'll just wait? :-)

Thanks!

Nico


>
>
>>  The check itself looks a bit odd -- usually, one would check whether a
>>>>> certain expression is valid (something like putting
>>>>> sizeof(declval<Type>().reportMemoryUsage()) into your SFINAE context)
>>>>> rather than checking whether a member with the given name exists -- but
>>>>> assuming it's getting the effect you want, I'll have a think about a more
>>>>> reliable way to perform the test.
>>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130418/35fafbcd/attachment.html>