[cfe-commits] r147023 - in /cfe/trunk: lib/Sema/SemaExpr.cpp lib/Sema/SemaTemplateInstantiateDecl.cpp test/PCH/chain-cxx.cpp test/SemaTemplate/instantiate-declref-ice.cpp

[Enea Zaffanella]

Il 02/01/2012 23:53, Richard Smith ha scritto:
> On Mon, January 2, 2012 21:37, Enea Zaffanella wrote:
>> Il 21/12/2011 01:25, Richard Smith ha scritto:
>>> Author: rsmith
>>> Date: Tue Dec 20 18:25:33 2011
>>> New Revision: 147023
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=147023&view=rev
>>> Log:
>>> C++ constant expression handling: eagerly instantiate static const integral
>>> data members of class templates so that their values can be used in ICEs.
>>> This
>>> required reverting r105465, to get such instantiated members to be included
>>> in serialized ASTs.
>>
>> Hi Richard.
>>
>> I noticed only now that your commit was reverting r105465:
>>
>>> --- cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp	2010/06/04 08:34:32
>>> 105464
>>> +++ cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp	2010/06/04 09:35:39
>>> 105465
>>> @@ -373,7 +373,8 @@
>>> SemaRef.CheckVariableDeclaration(Var, Previous, Redeclaration);
>>>
>>>
>>> if (D->isOutOfLine()) { -    D->getLexicalDeclContext()->addDecl(Var);
>>> +    if (!D->isStaticDataMember())
>>> +      D->getLexicalDeclContext()->addDecl(Var);
>>> Owner->makeDeclVisibleInContext(Var);
>>> } else {
>>> Owner->addDecl(Var);
>>>
>>
>>
>> For reference, the change in r105465 was discussed on the cfe-dev list
>> in this thread:
>>
>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-June/009231.html
> 
> Firstly, no tests were introduced by r105465, and none broke when I reverted
> it; do you have a case where this matters? (Is this important for some
> external tool using libclang, perhaps?)
> 
>> where it was agreed that "Implicit instantiations should not show up in the
>> translation unit context."
> 
> Well, they don't, necessarily -- they show up in the declaration context where
> the data member template was defined. That doesn't seem obviously wrong to me,
> but maybe it is. I asked Doug about this on IRC before making the change, and
> he had no immediate objections.

It is not "obviously wrong" (and let me say that you are doing a great
job in improving the evaluation of constants in clang); however, this
specific change is introducing an inconsistency in the handling of
implicit instantiations of templates.

Basically, most applications visiting the AST are going to be affected,
because they will now stumble on "unexpected" implicit instantiations of
static data member definitions. The fact that implicit instantiations do
not show up in the syntactic (TU / namespace / class) context was an
invariant that was likely exploited by most of these clients (e.g.,
pretty printers and several other source-code based tools).

Obviously, these applications could be fixed to match the new state of
things, but is this "correction" really needed and appropriate?
As far as I can tell, there is no compelling reason to introduce such an
inconsistency.

>> I understand from what you wrote above that these implicit
>> instantiations need to occur in the serialized AST. Have you considered the
>> possibility of replacing the (inline) static member declaration inside the
>> implicit instantiation of the class template? This is basically what is done
>> when instantiating out-of-line definitions of class methods, they appear to be
>> "inline" (but not in the formal sense) method definition.
> 
> Right, in the case of a function template, we add a definition to an
> already-instantiated declaration, which (for a member function) lives inside
> the class template. It would be strange to mark an in-class declaration of a
> static data member as being a definition, though, since definitions of static
> data members can't appear within a class.
> 
> - Richard

Strictly speaking, the rule you are mentioning applies to written code
(i.e., syntax), it is not really meant for code obtained by implicit
instantiations. Being non-syntactic, implicit instantiation are allowed
to sometimes deviate from the usual syntactic rules. For instance, as I
mentioned above, the instantiations of out-of-line definitions of
methods occur *inside* the instantiated class, but are anyway flagged to
be out-of-line.

We have the following code for methods:

01390 bool CXXMethodDecl::hasInlineBody() const {
01391   // If this function is a template instantiation, look at the
template from
01392   // which it was instantiated.
01393   const FunctionDecl *CheckFn = getTemplateInstantiationPattern();
01394   if (!CheckFn)
01395     CheckFn = this;
01396
01397   const FunctionDecl *fn;
01398   return CheckFn->hasBody(fn) && !fn->isOutOfLine();
01399 }

and the following for static data members:

01304 bool VarDecl::isOutOfLine() const {
01305   if (Decl::isOutOfLine())
01306     return true;
01307
01308   if (!isStaticDataMember())
01309     return false;
01310
01311   // If this static data member was instantiated from a static
data member of
01312   // a class template, check whether that static data member was
defined
01313   // out-of-line.
01314   if (VarDecl *VD = getInstantiatedFromStaticDataMember())
01315     return VD->isOutOfLine();
01316
01317   return false;
01318 }


The same choice was done for out-of-line definitions of class members of
a class template, as you can see by dumping the following fragment:
========================
template <typename T>
struct A {
  struct Inner;
};

template <typename T>
struct A<T>::Inner {
  int a;
};

void foo() {
  A<int>::Inner inner;
}
========================


To summarize, *if* there are no other reasons motivating the change
above, we would appreciate if the implicit instantiations of definitions
of static data members find their place inside the implicit
instantiation of their class, like all the other forms of implicit
instantiation.

Enea.