Instantiation of static class members may be a source of user misunderstanding,
especially in the case of using modules, PR24425 describes one of such examples.
This patch implements warning if compiler tried to make implicit instantiation
but could not find template definition. The implementation follows discussion
of http://reviews.llvm.org/D12326.
Details
Diff Detail
Event Timeline
Are you planning on extending this to also cover templated functions?
include/clang/Basic/DiagnosticGroups.td | ||
---|---|---|
78 | undefined rather than unavailable? "unavailable" means something different in Clang. | |
include/clang/Basic/DiagnosticSemaKinds.td | ||
3888 | not found -> not defined? The template is found, or the code would be ill-formed. Also, 'corresponding template' isn't quite right; in S<int>::m, the template is S, which must be defined. Maybe you could map to the pattern declaration for the instantiation, and warn that it isn't defined, so we'd say something like "instantiation of 'S<int>::m' required here, but 'S<T>::m' is not defined" | |
lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
4103 | Explicit instantiation declarations are a C++11 feature. Should we suppress this warning, or at least the note, in C++98? |
Updated patch according to review notes.
The patch adds diagnostics for undefined function templates, in a similar way
as for template variables. These two warnings are classified into different groups.
Warnings on undefined variable templates are enabled by default. This is more
error-prone case, existence of PR24425 may be considered as an evidence. Warnings
on undefined function templates are off by default. If it is enabled,
about 80 tests require update.
References to template declarations can now be printed in two ways. The usual way
is to print only names, such as 'C1::C2', it is compact and convenient when the
reference occurs inside template definition. The patch adds new format, in which
template parameters are also printed, like 'C1<T>::C2<T1,N1>', it can be more
convenient if the template is referenced from outside.
I would prefer to avoid adding the %qt format if we can. But if we do provide it, the template parameter names we use should match what was written in the declaration of the template itself.
lib/AST/Decl.cpp | ||
---|---|---|
1423 ↗ | (On Diff #53547) | Presumably you mean C1<T, Val>::C2<T1, TC>::meth<T2>? |
1447 ↗ | (On Diff #53547) | Please don't invent a name here. If you really want this formatting for template names, please query the TemplateDecl to find the names that were used when declaring its parameters. |
1464 ↗ | (On Diff #53547) | This doesn't make sense: a template template argument is passed as a template, but adding <> would make it a type instead. Remove this. |
lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
3611 | Please produce a note pointing at the declaration of the undefined template. With a bit of rewording the diagnostics, I think you can remove the need for the %qt format entirely: warning: instantiation for 'X<int>::f<double>' is required but no definition is available X<int>::f(0.0); ^ note: forward declaration of template entity is here template<typename T> void f(T); ^ note: add an explicit instantiation declaration to suppress this warning if 'X<int>::f<double>' is explicitly instantiated in another translation unit |
Updated patch
Get rid of '%qt' format, instead print template parameters always. It requires
about 10 tests to be fixed. However printing parameters still can be suppressed,
it is required for proper work of AST matchers, - a matcher may want to match
names like 'Y::f' where 'Y' is a template. Template parameters are printed as
they were written in template declarations.
I have a lingering concern with this style of formatting, because it risks making our diagnostics (more) ambiguous. Consider this:
template<typename T> struct X {}; struct T {}; X<T> xt;
If we give a diagnostic talking about X<T>, it's ambiguous whether we're referring to the X template itself or the concrete type of xt. That said, the actual diagnostic changes here do look like improvements, particularly because the template parameter names we're using are typically in scope at the point where we issue the diagnostic (so there is no ambiguity). On the whole, I think the risk of ambiguity is less than the benefit here.
lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
---|---|---|
3611 | I would still like this note to be added to the diagnostic. | |
test/OpenMP/parallel_ast_print.cpp | ||
135 | This looks like an undesirable change. Can you turn off PrintTemplateArguments by default and turn it on only for %t in diagnostics? |
Updated patch
Changed text of messages as proposed by reviewer. As a result, printing
template parameters is not needed anymore, related code is removed.
undefined rather than unavailable? "unavailable" means something different in Clang.