<div dir="ltr"><div class="gmail_extra">On Thu, Apr 18, 2013 at 4:46 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="im">On Thu, Apr 18, 2013 at 1:15 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>On Thu, Apr 18, 2013 at 11:10 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>On Thu, Apr 18, 2013 at 10:50 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Filed PR15783, thanks.</div><div><br>
</div>
The WebKit code was added here and seems to be doing what the people who wrote it want:<div><a href="https://bugs.webkit.org/show_bug.cgi?id=105026" target="_blank">https://bugs.webkit.org/show_bug.cgi?id=105026</a><br>
</div><div><a href="https://bugs.webkit.org/attachment.cgi?id=179485&action=review" target="_blank">https://bugs.webkit.org/attachment.cgi?id=179485&action=review</a></div></div></blockquote><div><br></div></div>
<div>
Looks like it's trying to determine whether this will compile:</div><div><br></div><div><div>template <></div><div>template <typename T></div><div>void MemoryInstrumentation::InstrumentationSelector<true>::reportObjectMemoryUsage(const T* object, MemoryObjectInfo* memoryObjectInfo)</div>
<div>{</div><div> object->reportMemoryUsage(memoryObjectInfo);</div><div>}</div></div><div><br></div><div>For that, it should really be doing SFINAE on the "object->reportMemoryUsage(memoryObjectInfo)" expression.</div>
</div></div></div></blockquote><div><br></div></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div><div>Should be. Which compilers do you need this to work on? :)</div></div></div></div></blockquote><div><br></div><div style>Nothing too exotic: gcc4.6+, clang ~trunk-ish, msvs2010+</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><div class="gmail_extra">
<div class="gmail_quote">
On Thu, Apr 18, 2013 at 10:43 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div>On Thu, Apr 18, 2013 at 10:09 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br></div></div><div class="gmail_extra">
<div class="gmail_quote"><div><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Richard,<div><br></div><div>this breaks compilation of this bit of code in WebKit, which does metaprogramming to check if a class implements a given method:</div>
<div><br></div><div><div>template <typename Type> class IsInstrumented {</div>
<div> class yes { char m; };</div><div> class no { yes m[2]; };</div><div> struct BaseMixin {</div><div> void reportMemoryUsage() const {}</div><div> };</div><div> struct Base : public Type, public BaseMixin {</div>
<div> };</div><div> template <typename T, T t> class Helper {</div><div> };</div><div> template <typename U></div><div> static no</div><div> deduce(U *, Helper<void(BaseMixin::*)() const, &U::reportMemoryUsage> * = 0);</div>
<div> static yes deduce(...);</div><div>public:</div><div> static const bool result = sizeof(yes) == sizeof(deduce((Base *)(0)));</div><div>};</div><div><br></div><div>template <typename T> bool hasReportMemoryUsage(const T *object) {</div>
<div> return IsInstrumented<T>::result;</div><div>}</div><div><br></div><div>class Timer {</div><div> void operator delete(void *p) {}</div><div>public:</div><div> virtual ~Timer();</div><div>};</div><div>bool f() {</div>
<div> Timer m_cacheTimer;</div><div> return hasReportMemoryUsage(&m_cacheTimer);</div><div>}</div></div><div><br></div><div><br></div><div>The error message looks like this:</div><div><br></div><div>
<div>$ clang -c repro.ii -std=gnu++11 # works in older clang</div><div>$ third_party/llvm-build/Release+Asserts/bin/clang -c repro.ii -std=gnu++11</div><div>repro.ii:7:10: error: deleted function '~Base' cannot override a non-deleted function</div>
<div> struct Base : public Type, public BaseMixin {</div><div> ^</div><div>repro.ii:13:51: note: in instantiation of member class 'IsInstrumented<Timer>::Base' requested here</div><div> deduce(U *, Helper<void(BaseMixin::*)() const, &U::reportMemoryUsage> * = 0);</div>
<div> ^</div><div>repro.ii:13:3: note: while substituting deduced template arguments into function template 'deduce' [with U = IsInstrumented<Timer>::Base]</div>
<div> deduce(U *, Helper<void(BaseMixin::*)() const, &U::reportMemoryUsage> * = 0);</div><div> ^</div><div>repro.ii:20:10: note: in instantiation of template class 'IsInstrumented<Timer>' requested here</div>
<div> return IsInstrumented<T>::result;</div><div> ^</div><div>repro.ii:30:10: note: in instantiation of function template specialization 'hasReportMemoryUsage<Timer>' requested here</div><div>
return hasReportMemoryUsage(&m_cacheTimer);</div><div> ^</div><div>repro.ii:26:11: note: overridden virtual function is here</div><div> virtual ~Timer();</div><div> ^</div><div>1 error generated.</div>
<div><br></div></div><div><br></div><div>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).</div></div></blockquote><div><br></div></div></div><div>
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'.</div>
</div></div></div></blockquote></div></div></div></div></blockquote></div></div></div></div></div></blockquote></div></div></div></blockquote><div><br></div></div></div><div>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:</div>
<div><span style="color:rgb(80,0,80)"><br></span></div><div><font color="#500050">struct A { void operator delete(void*, long); }; struct B : protected A { virtual ~B(); }; struct C : B {};</font><br></div><div>
<font color="#500050"><br></font></div><div><font color="#500050">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.</font></div>
<div><font color="#500050"><br></font></div><div><span style="color:rgb(80,0,80)">For a C++98 version of the same behavior, we reject this:</span><br></div><div>
<font color="#500050"><br></font></div><div><font color="#500050">struct A { void operator delete(void*, long); }; struct B : protected A { virtual ~B(); }; struct C : B { ~C() { operator delete(0, 0); } };<br></font></div>
<div><font color="#500050"><br></font></div><div><font color="#500050">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.</font></div>
<div><font color="#500050"><br></font></div><div><font color="#500050">The WebKit bug was fixed here (to the point where Clang should accept it):</font></div><div><font color="#500050"><br></font></div><div><font color="#500050"><a href="http://trac.webkit.org/changeset/147751/trunk/Source/WebCore/platform/Timer.h" target="_blank">http://trac.webkit.org/changeset/147751/trunk/Source/WebCore/platform/Timer.h</a></font></div>
</div></div></div></blockquote><div><br></div><div style>Yes, but it seems a bit unfortunate that the SFINA implementation detail prevents private inheritance many files away.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<div><span style="color:rgb(80,0,80)"> </span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div>It compiles fine if I give Base an explicit destructor. Is this the right fix?</div>
</div>
</blockquote><div><br></div></div><div>It's inelegant, but yes, that should work (unless you use this with a final class or similar...).</div></div></div></div></blockquote></div></div></blockquote></div></div></div>
</blockquote></div></div></div></div></blockquote><div><br></div></div><div>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.</div>
</div></div></div></blockquote><div><br></div><div style>Ok, then I'll just wait? :-)</div><div style><br></div><div style>Thanks!</div><div style><br></div><div style>Nico</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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.</div>
</div></div></div>
</blockquote></div><br></div>
</blockquote></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div></div>