Page MenuHomePhabricator

Warn if variable cannot be implicitly instantiated
ClosedPublic

Authored by sepavloff on Jan 21 2016, 12:33 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

sepavloff updated this revision to Diff 45494.Jan 21 2016, 12:33 AM
sepavloff retitled this revision from to Warn if variable cannot be implicitly instantiated.
sepavloff updated this object.
sepavloff added a reviewer: rsmith.
sepavloff added subscribers: cfe-commits, silvas.

Any feedback?

Thanks,
--Serge

rsmith edited edge metadata.Mar 18 2016, 10:58 AM

Are you planning on extending this to also cover templated functions?

include/clang/Basic/DiagnosticGroups.td
78 ↗(On Diff #45494)

undefined rather than unavailable? "unavailable" means something different in Clang.

include/clang/Basic/DiagnosticSemaKinds.td
3826 ↗(On Diff #45494)

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
3909 ↗(On Diff #45494)

Explicit instantiation declarations are a C++11 feature. Should we suppress this warning, or at least the note, in C++98?

sepavloff updated this revision to Diff 53547.Apr 13 2016, 6:08 AM
sepavloff edited edge metadata.

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 ↗(On Diff #53547)

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
sepavloff updated this revision to Diff 53853.Apr 15 2016, 12:38 AM

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 ↗(On Diff #53853)

I would still like this note to be added to the diagnostic.

test/OpenMP/parallel_ast_print.cpp
135 ↗(On Diff #53853)

This looks like an undesirable change. Can you turn off PrintTemplateArguments by default and turn it on only for %t in diagnostics?

sepavloff updated this revision to Diff 54085.Apr 18 2016, 11:09 AM

Updated patch

Changed text of messages as proposed by reviewer. As a result, printing
template parameters is not needed anymore, related code is removed.

rsmith accepted this revision.Apr 18 2016, 11:18 AM
rsmith edited edge metadata.

Thanks!

This revision is now accepted and ready to land.Apr 18 2016, 11:18 AM
This revision was automatically updated to reflect the committed changes.