<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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 ----------------------------------------------------</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_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_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_OPERAND_CLASS</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">------ lithium.cc----------------------------------------------------</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>::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_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 -----------------------------------------------</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(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 -----------------------------------------------------</font></div><div><font face="monospace, monospace">g++ lithium.cc lithium-x64.cc</font></div><div><font face="monospace, monospace">---------------------------------------------------------------------</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', 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_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>The use case of Singleton<T> I gave wasn't complete. It was more like this:<br></div><div><br></div><div><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><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><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">-- Sean Silva</div><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 class=""><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 class=""><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/lithium.h:322:45: error: instantiation of variable 'v8::internal::LSubKindOperand<v8::internal::LOperand::Kind::DOUBLE_STACK_SLOT, 128>::cache' required here, but no definition is available [-Werror,-Wundefined-var-template]</div><span><div> if (index < kNumCachedOperands) return &cache[index];</div></span><div> ^</div><div>../../v8/src/crankshaft/x64/lithium-x64.cc:334:30: note: in instantiation of member function 'v8::internal::LSubKindOperand<v8::internal::LOperand::Kind::DOUBLE_STACK_SLOT, 128>::Create' requested here</div><div> return LDoubleStackSlot::Create(index, zone());</div><div> ^</div><div>../../v8/src/crankshaft/lithium.h:335:27: note: forward declaration of template entity is here</div><div> static LSubKindOperand* cache;</div><div> ^</div><div>../../v8/src/crankshaft/lithium.h:322:45: note: add an explicit instantiation declaration to suppress this warning if 'v8::internal::LSubKindOperand<v8::internal::LOperand::Kind::DOUBLE_STACK_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></div><br></div></div>