On Thu, Apr 18, 2013 at 4:46 PM, Richard Smith <[email protected]>wrote:
> On Thu, Apr 18, 2013 at 1:15 PM, Nico Weber <[email protected]> wrote: > >> On Thu, Apr 18, 2013 at 11:10 AM, Richard Smith <[email protected]>wrote: >> >>> On Thu, Apr 18, 2013 at 10:50 AM, Nico Weber <[email protected]>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 > <[email protected]>wrote: >>>> >>>>> On Thu, Apr 18, 2013 at 10:09 AM, Nico Weber <[email protected]>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. >>>>> >>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
