[cfe-commits] r105882 - in /cfe/trunk: include/clang/AST/Decl.h lib/AST/Decl.cpp lib/Sema/SemaDecl.cpp lib/Sema/SemaTemplate.cpp

[Douglas Gregor]

On Jun 15, 2010, at 7:34 AM, Abramo Bagnara wrote:

> Il 14/06/2010 23:02, Douglas Gregor ha scritto:
>> 
>> On Jun 12, 2010, at 1:15 AM, Abramo Bagnara wrote:
>> 
>>> Author: abramo
>>> Date: Sat Jun 12 03:15:14 2010
>>> New Revision: 105882
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=105882&view=rev
>>> Log:
>>> Added template parameters info for out-of-line definitions of class template methods.
>>> 
>>> Modified:
>>>   cfe/trunk/include/clang/AST/Decl.h
>>>   cfe/trunk/lib/AST/Decl.cpp
>>>   cfe/trunk/lib/Sema/SemaDecl.cpp
>>>   cfe/trunk/lib/Sema/SemaTemplate.cpp
>>> 
>>> Modified: cfe/trunk/include/clang/AST/Decl.h
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=105882&r1=105881&r2=105882&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/AST/Decl.h (original)
>>> +++ cfe/trunk/include/clang/AST/Decl.h Sat Jun 12 03:15:14 2010
>>> @@ -28,6 +28,8 @@
>>> class Stmt;
>>> class CompoundStmt;
>>> class StringLiteral;
>>> +class NestedNameSpecifier;
>>> +class TemplateParameterList;
>>> class TemplateArgumentList;
>>> class MemberSpecializationInfo;
>>> class FunctionTemplateSpecializationInfo;
>>> @@ -364,15 +366,42 @@
>>>  static bool classofKind(Kind K) { return K >= firstValue && K <= lastValue; }
>>> };
>>> 
>>> +/// QualifierInfo - A struct with extended info about a syntactic
>>> +/// name qualifier, to be used for the case of out-of-line declarations.
>>> +struct QualifierInfo {
>>> +  /// NNS - The syntactic name qualifier.
>>> +  NestedNameSpecifier *NNS;
>>> +  /// NNSRange - The source range for the qualifier.
>>> +  SourceRange NNSRange;
>>> +  /// NumTemplParamLists - The number of template parameter lists
>>> +  /// that were matched against the template-ids occurring into the NNS.
>>> +  unsigned NumTemplParamLists;
>>> +  /// TemplParamLists - A new-allocated array of size NumTemplParamLists,
>>> +  /// containing pointers to the matched template parameter lists.
>>> +  TemplateParameterList** TemplParamLists;
>>> +
>>> +  /// Default constructor.
>>> +  QualifierInfo()
>>> +    : NNS(0), NNSRange(), NumTemplParamLists(0), TemplParamLists(0) {}
>>> +  /// setTemplateParameterListsInfo - Sets info about matched template
>>> +  /// parameter lists.
>>> +  void setTemplateParameterListsInfo(unsigned NumTPLists,
>>> +                                     TemplateParameterList **TPLists);
>>> +  /// Destructor: frees the array of template parameter lists pointers.
>>> +  ~QualifierInfo() { delete[] TemplParamLists; }
>> 
>> The template parameter lists should be stored in memory allocated via the ASTContext, not on the normal heap. AST or Sema will need to copy the TemplateParameterList pointers into ASTContext-allocated memory, and rather than have a destructor, QualifierInfo should have a Destroy method that deallocates that memory.
>> 
>> Everything else looks great!
> 
> We have had a try at it, but got something unsatisfactory.
> 
> There is something still unclear to us in the clang design for resource
> handling
> (for instance, why TemplateParameterList has no Destroy method?)

We allocate everything that is intended to be in the AST via the ASTContext, which (in the fast path enabled by -disable-free) is allocated via a BumpPtrAllocator. That memory is deallocated in one fast step when the ASTContext is destroyed. The Destroy methods, which are never called in the -disable-free path, are used to clean up memory in the "slow" path.

However, we don't really use the Destroy methods consistently, so they're in a state of disarray. There has been some discussion of eliminating them entirely (and committing to the BumpPtrAllocator model fully), but I don't think we're ready to commit to that (or to making all of the Destroy methods perfect) yet.

> We would appreciate if someone more expert in these issues does it this
> time ... and we will learn for the future.

Okay. It's in r106008.

	- Doug