r266719 - Warn if function or variable cannot be implicitly instantiated
Serge Pavlov via cfe-commits
cfe-commits at lists.llvm.org
Thu May 19 12:13:39 PDT 2016
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()`. In this case
`Singleton<Foo>::getInstance()` 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.
Probably it is more convenient to put the template for the static member to
header file too:
template <typename T>
T *Singleton<T>::instance = nullptr;
In this case both the method and corresponding static member are
instantiated implicitly by compiler, no warning occurs.
Can it be an acceptable solution?
If there is intention to limit `Singleton` to some selected types, explicit
instantiation declaration may help. Adding the declarations like:
extern template Foo *Singleton<Foo>::instance;
prevents from warnings.
Or you think that the message should propose moving static member
definition into header file as well?
Thanks,
--Serge
2016-05-19 9:15 GMT+06:00 Sean Silva <chisophugis at gmail.com>:
>
>
> On Thu, Apr 21, 2016 at 12:44 AM, Serge Pavlov <sepavloff at gmail.com>
> wrote:
>
>> Let me demonstrate the problem using excerpt from v8 sources:
>>
>> ------ lithium.h ----------------------------------------------------
>> template <int kOperandKind, int kNumCachedOperands>
>> struct LSubKindOperand {
>> static int* Create(int index) { return &cache[index]; }
>> static void SetUpCache();
>> static int* cache;
>> };
>>
>> struct LOperand {
>> static void SetUpCaches();
>> };
>>
>> #define LITHIUM_OPERAND_LIST(V) \
>> V(DoubleRegister, 1, 16)
>>
>> #define LITHIUM_TYPEDEF_SUBKIND_OPERAND_CLASS(name, type, number) \
>> typedef LSubKindOperand<type, number> L##name;
>> LITHIUM_OPERAND_LIST(LITHIUM_TYPEDEF_SUBKIND_OPERAND_CLASS)
>> /* Expands to: typedef LSubKindOperand<1, 16> LDoubleRegister; */
>> #undef LITHIUM_TYPEDEF_SUBKIND_OPERAND_CLASS
>>
>> ------ lithium.cc----------------------------------------------------
>> #include "lithium.h"
>>
>> template<int kOperandKind, int kNumCachedOperands>
>> int* LSubKindOperand<kOperandKind, kNumCachedOperands>::cache = 0;
>>
>> template<int kOperandKind, int kNumCachedOperands>
>> void LSubKindOperand<kOperandKind, kNumCachedOperands>::SetUpCache() {
>> cache = new int[kNumCachedOperands];
>> }
>>
>> void LOperand::SetUpCaches() {
>> #define LITHIUM_OPERAND_SETUP(name, type, number) L##name::SetUpCache();
>> LITHIUM_OPERAND_LIST(LITHIUM_OPERAND_SETUP)
>> /* Expands to: LDoubleRegister::SetUpCache(); */
>> #undef LITHIUM_OPERAND_SETUP
>> }
>>
>> ------ lithium-x64.cc -----------------------------------------------
>> #include "lithium.h"
>>
>> int* GetNextSpillSlot(int kind) {
>> return LSubKindOperand<1,16>::Create(0);
>> }
>>
>> int main(int argc, char *argv[]) {
>> return 0;
>> }
>>
>> ------ build.sh -----------------------------------------------------
>> g++ lithium.cc lithium-x64.cc
>> ---------------------------------------------------------------------
>>
>> 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.
>>
>> 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.
>>
>> 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.
>>
>> 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.
>>
>> 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 https://llvm.org/bugs/show_bug.cgi?id=24425 is just one
>> example. The change introduced in r266719 attempts to provide user with
>> some assistance in these cases.
>>
>> 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.
>>
> The use case of Singleton<T> I gave wasn't complete. It was more like this:
>
> template <typename T>
> struct Singleton {
> ... other stuff ...
> static T *getInstance() { return Singleton<T>::instance; }
> private:
> static T *instance;
> };
>
> Making the method private to a .cpp file would defeat the entire purpose
> of having the class.
>
> -- Sean Silva
>
>
>> 2016-04-21 5:36 GMT+06:00 Sean Silva <chisophugis at gmail.com>:
>>
>>>
>>>
>>> On Tue, Apr 19, 2016 at 7:28 AM, Nico Weber via cfe-commits <
>>> cfe-commits at lists.llvm.org> wrote:
>>>
>>>> (sorry, accidentally sent this mid-mail)
>>>>
>>>> ../../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]
>>>> if (index < kNumCachedOperands) return &cache[index];
>>>> ^
>>>> ../../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
>>>> return LDoubleStackSlot::Create(index, zone());
>>>> ^
>>>> ../../v8/src/crankshaft/lithium.h:335:27: note: forward declaration of
>>>> template entity is here
>>>> static LSubKindOperand* cache;
>>>> ^
>>>> ../../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
>>>> if (index < kNumCachedOperands) return &cache[index];
>>>> ^
>>>>
>>>> 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.
>>>>
>>>
>>> Yes, the diagnostics here can be substantially improved I think.
>>>
>>
>> Do you have any thoughts how the diagnostics could be improved? Any ideas
>> are welcome.
>>
>> Thanks,
>> --Serge
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160520/ab0fdef7/attachment.html>
More information about the cfe-commits
mailing list