<div dir="ltr">With 3.9 coming around, I'd like to suggest that we pull this warning from 3.9 and maybe trunk. It sounds like Sean found it only possible to deploy this warning since they already had a workaround for a different compiler bug (!). In Chromium, we can't enable this warning since one of our (admittedly dubiously designed) template classes in a foundational library requires people to have an explicit instantiation of their downstream classes in a client cc file. Fixing this warning would mean giving the h file in the foundational library forward declarations of all clients of the template.<div><br></div><div>The motivation for this warning was PR24425, which is something that's only relevant with modules enabled. Maybe the warning should only fire with modules. But as-is, the warning warns about something that isn't really a problem in practice, and it's difficult to fix (and as said, fixing it has few benefits). I don't think it's at the bar we have for clang warnings.</div><div><br></div><div><div>Is there anyone who has deployed this warning successfully on a larger codebase? Examples of real bugs it found?</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 20, 2016 at 12:14 AM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, May 19, 2016 at 12:13 PM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>In this case moving implementation of `Singleton<T>::getInstance()` into a .cpp file would prevent compiler from instantiation of the method body when it sees `Singleton<Foo>::getInstance()<wbr>`. In this case `Singleton<Foo>::getInstance()<wbr>` must be instantiated in some source file either explicitly or implicitly. If inline implementation for `Singleton<T>::getInstance()` is provided in the header, where `Singleton<T>` is defined, the method body is instantiated implicitly when it is required. But the static field `instance` referenced in the method still must be instantiated in some source file explicitly or implicitly. So from viewpoint of convenience, moving the implementation of template `Singleton<T>::getInstance()` into source file does not look as more inflexible solution. </div><div><br></div><div>Probably it is more convenient to put the template for the static member to header file too:</div><div><br></div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div>template <typename T></div><div>T *Singleton<T>::instance = nullptr;</div></blockquote><div><br></div><div>In this case both the method and corresponding static member are instantiated implicitly by compiler, no warning occurs.</div><div>Can it be an acceptable solution?</div></div></blockquote><div><br></div></span><div>I think that is what makes the most sense in this scenario (and it simplifies things for clients of the library).</div><div>Unfortunately, for the library that was producing this warning they already required clients to provide explicit instantiations in a .cpp file (they had a macro like `DEFINE_SINGLETON_INSTANCE(T)` which a user would place in a .cpp file to instantiate Singleton<T>::instance for their type).</div><div><br></div><div>So fixing this warning like this would have a compatibility concern.</div><div><br></div><div>Thankfully, it seems like some compilers (not clang) have a bug in that they will emit Singleton<T>::instance any time that Singleton<T> is instantiated unless they have already seen an explicit instantiation declaration of Singleton<Foo>::instance in a header, so this library already had (under an #if) explicit instantiations for Singleton<Foo>::instance for all classes that needed it in order to work around this compiler bug. So in the end I fixed this by just enabling those ifdefs so that clang would see the Singleton<Foo>::instance explicitly declared in the header.</div><div>(This is sort of a strange coincidence, but it worked out nicely)</div><span class=""><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div><div>If there is intention to limit `Singleton` to some selected types, explicit instantiation declaration may help. Adding the declarations like:</div><div><br></div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div>extern template Foo *Singleton<Foo>::instance;</div></blockquote><div><br></div><div>prevents from warnings.</div><div><br></div><div>Or you think that the message should propose moving static member definition into header file as well?</div></div></blockquote><div><br></div></span><div>I'm not sure, but I think that the current diagnostics are not very actionable. Hopefully google will lead users to your description in this thread. I wish we had something like <a href="https://doc.rust-lang.org/error-index.html" target="_blank">https://doc.rust-lang.<wbr>org/error-index.html</a> where we could officially provide a more in-depth explanation similar to your posts in this thread.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div></div><div class="gmail_extra"><br clear="all"><div><div>Thanks,<br>--Serge<br></div></div><div><div class="m_-5423972339030900266h5">
<br><div class="gmail_quote">2016-05-19 9:15 GMT+06:00 Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Thu, Apr 21, 2016 at 12:44 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Let me demonstrate the problem using excerpt from v8 sources:</div><div><br></div><div><font face="monospace, monospace">------ lithium.h ------------------------------<wbr>----------------------</font></div><div><font face="monospace, monospace">template <int kOperandKind, int kNumCachedOperands></font></div><div><font face="monospace, monospace">struct LSubKindOperand {</font></div><div><font face="monospace, monospace"> static int* Create(int index) { return &cache[index]; }</font></div><div><font face="monospace, monospace"> static void SetUpCache();</font></div><div><font face="monospace, monospace"> static int* cache;</font></div><div><font face="monospace, monospace">};</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">struct LOperand {</font></div><div><font face="monospace, monospace"> static void SetUpCaches();</font></div><div><font face="monospace, monospace">};</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">#define LITHIUM_OPERAND_LIST(V) \</font></div><div><font face="monospace, monospace"> V(DoubleRegister, 1, 16)</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">#define LITHIUM_TYPEDEF_SUBKIND_<wbr>OPERAND_CLASS(name, type, number) \</font></div><div><font face="monospace, monospace">typedef LSubKindOperand<type, number> L##name;</font></div><div><font face="monospace, monospace">LITHIUM_OPERAND_LIST(LITHIUM_<wbr>TYPEDEF_SUBKIND_OPERAND_CLASS)</font></div><div><font face="monospace, monospace">/* Expands to: typedef LSubKindOperand<1, 16> LDoubleRegister; */</font></div><div><font face="monospace, monospace">#undef LITHIUM_TYPEDEF_SUBKIND_<wbr>OPERAND_CLASS</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">------ lithium.cc--------------------<wbr>------------------------------<wbr>--</font></div><div><font face="monospace, monospace">#include "lithium.h"</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">template<int kOperandKind, int kNumCachedOperands></font></div><div><font face="monospace, monospace">int* LSubKindOperand<kOperandKind, kNumCachedOperands>::cache = 0;</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">template<int kOperandKind, int kNumCachedOperands></font></div><div><font face="monospace, monospace">void LSubKindOperand<kOperandKind, kNumCachedOperands>::<wbr>SetUpCache() {</font></div><div><font face="monospace, monospace"> cache = new int[kNumCachedOperands];</font></div><div><font face="monospace, monospace">}</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">void LOperand::SetUpCaches() {</font></div><div><font face="monospace, monospace">#define LITHIUM_OPERAND_SETUP(name, type, number) L##name::SetUpCache();</font></div><div><font face="monospace, monospace"> LITHIUM_OPERAND_LIST(LITHIUM_<wbr>OPERAND_SETUP)</font></div><div><font face="monospace, monospace"> /* Expands to: LDoubleRegister::SetUpCache(); */</font></div><div><font face="monospace, monospace">#undef LITHIUM_OPERAND_SETUP</font></div><div><font face="monospace, monospace">}</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">------ lithium-x64.cc ------------------------------<wbr>-----------------</font></div><div><font face="monospace, monospace">#include "lithium.h"</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">int* GetNextSpillSlot(int kind) {</font></div><div><font face="monospace, monospace"> return LSubKindOperand<1,16>::Create(<wbr>0);</font></div><div><font face="monospace, monospace">}</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">int main(int argc, char *argv[]) {</font></div><div><font face="monospace, monospace"> return 0;</font></div><div><font face="monospace, monospace">}</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">------ build.sh ------------------------------<wbr>-----------------------</font></div><div><font face="monospace, monospace">g++ lithium.cc lithium-x64.cc</font></div><div><font face="monospace, monospace">------------------------------<wbr>------------------------------<wbr>---------</font></div><div><br></div><div>When compiler generates code for 'GetNextSpillSlot' it implicitly instantiates the method 'Create'. It can do it as definition of the template entity 'LSubKindOperand::Create' is available from lithium.h. Then it attempts to instantiate static field 'LSubKindOperand::cache', as it is used in the body of just instantiated method 'Create'. But definition of this template entity is not available while compiling lithium-x64.cc. Failure to implicitly instantiate a template is not an error, but this template must be instantiated somewhere else otherwise linker founds undefined references.</div><div><br></div><div>Indeed, 'LSubKindOperand<1,16>::cache' is instantiated while compiling lithium.cc. Method 'LOperand::SetUpCaches' is not a template, it references 'LSubKindOperand::SetUpCache', which in turn uses 'LSubKindOperand::cache'. Definitions of these templates are available in lithium.cc, compiler instantiates them and the program builds successfully. This solution is fragile however, if lithium-x64.cc referenced 'LSubKindOperand<1,1>::Create'<wbr>, the build would break because in lithium.cc this specialization is not instantiated.</div><div><br></div><div>Putting templates definitions into source files rather than headers is similar to moving forward declarations into source files. It makes sense if user wants to encapsulate some functionality. In this example he should also move the definition 'LSubKindOperand::Create' to source file, just for the sake of encapsulation. This would prevent compiler from implicit instantiation of this method and thus from instantiation of the static field as well.</div><div><br></div><div>If encapsulation is not an aim, moving template definition into header seems to be more natural solution. It would allow for compiler to instantiate all entities implicitly.</div><div><br></div><div>Static variables of template classes are a source of confusion for some reason. Probably people tend to confuse them with static member template specializations. Anyway this type of errors is too widespread, the aforementioned <a href="https://llvm.org/bugs/show_bug.cgi?id=24425" target="_blank">https://llvm.org/bugs/show_<wbr>bug.cgi?id=24425</a> is just one example. The change introduced in r266719 attempts to provide user with some assistance in these cases.</div><div><br></div><div>I don't know details about Singleton<T>, but only existence of static fields should not produce the warning. There must be a reference to the field in some method defined inline. Solution could be similar: either the method that references the static field should be encapsulated into source file or the static field template should be exposed in header.</div></div></blockquote></div></div><div>The use case of Singleton<T> I gave wasn't complete. It was more like this:<br></div><div><br></div><div><span><div style="font-size:12.8px">template <typename T></div><div style="font-size:12.8px">struct Singleton {</div><div style="font-size:12.8px">... other stuff ...</div></span><div style="font-size:12.8px"> static T *getInstance() { return <span style="font-size:small">Singleton<T>::instance; }</span></div>
<div style="font-size:12.8px">private:</div><div style="font-size:12.8px"> static T *instance;<br>};</div></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">Making the method private to a .cpp file would defeat the entire purpose of having the class.</div><span><font color="#888888"><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">-- Sean Silva</div></font></span><span><div style="font-size:12.8px"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><span><div><br></div><div>2016-04-21 5:36 GMT+06:00 Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span>:<br></div></span><div class="gmail_extra"><div class="gmail_quote"><span><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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Apr 19, 2016 at 7:28 AM, Nico Weber via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br></span><span><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">(sorry, accidentally sent this mid-mail)<div><br></div><div><div>../../v8/src/crankshaft/<wbr>lithium.h:322:45: error: instantiation of variable 'v8::internal::<wbr>LSubKindOperand<v8::internal::<wbr>LOperand::Kind::DOUBLE_STACK_<wbr>SLOT, 128>::cache' required here, but no definition is available [-Werror,-Wundefined-var-<wbr>template]</div><span><div> if (index < kNumCachedOperands) return &cache[index];</div></span><div> ^</div><div>../../v8/src/crankshaft/x64/<wbr>lithium-x64.cc:334:30: note: in instantiation of member function 'v8::internal::<wbr>LSubKindOperand<v8::internal::<wbr>LOperand::Kind::DOUBLE_STACK_<wbr>SLOT, 128>::Create' requested here</div><div> return LDoubleStackSlot::Create(<wbr>index, zone());</div><div> ^</div><div>../../v8/src/crankshaft/<wbr>lithium.h:335:27: note: forward declaration of template entity is here</div><div> static LSubKindOperand* cache;</div><div> ^</div><div>../../v8/src/crankshaft/<wbr>lithium.h:322:45: note: add an explicit instantiation declaration to suppress this warning if 'v8::internal::<wbr>LSubKindOperand<v8::internal::<wbr>LOperand::Kind::DOUBLE_STACK_<wbr>SLOT, 128>::cache' is explicitly instantiated in another translation unit</div><span><div> if (index < kNumCachedOperands) return &cache[index];</div></span><div> ^</div></div><div><br></div><div>I don't understand what this warning wants to tell me, or what it wants me to do. What is this warning good for? Neither warning text nor commit message explain that.</div></div></blockquote><div><br></div></span><div>Yes, the diagnostics here can be substantially improved I think.</div></div></div></div></blockquote><div><br></div></span><div>Do you have any thoughts how the diagnostics could be improved? Any ideas are welcome.</div><div> </div><div>Thanks,</div><div>--Serge</div></div></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>