r260277 - Simplify and rename ASTMatchFinder's getCXXRecordDecl to make it more obvious

[Manuel Klimek via cfe-commits]

On Wed, Feb 10, 2016 at 10:04 AM Manuel Klimek <klimek at google.com> wrote:

> On Tue, Feb 9, 2016 at 10:03 PM Richard Smith via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: rsmith
>> Date: Tue Feb  9 14:59:05 2016
>> New Revision: 260277
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=260277&view=rev
>> Log:
>> Simplify and rename ASTMatchFinder's getCXXRecordDecl to make it more
>> obvious
>> what it's actually trying to do.
>>
>> Modified:
>>     cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
>>
>> Modified: cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp?rev=260277&r1=260276&r2=260277&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp (original)
>> +++ cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp Tue Feb  9 14:59:05 2016
>> @@ -744,46 +744,25 @@ private:
>>    MemoizationMap ResultCache;
>>  };
>>
>> -static CXXRecordDecl *getAsCXXRecordDecl(const Type *TypeNode) {
>> -  // Type::getAs<...>() drills through typedefs.
>> -  if (TypeNode->getAs<DependentNameType>() != nullptr ||
>> -      TypeNode->getAs<DependentTemplateSpecializationType>() != nullptr
>> ||
>> -      TypeNode->getAs<TemplateTypeParmType>() != nullptr)
>> -    // Dependent names and template TypeNode parameters will be matched
>> when
>> -    // the template is instantiated.
>> -    return nullptr;
>> -  TemplateSpecializationType const *TemplateType =
>> -      TypeNode->getAs<TemplateSpecializationType>();
>> -  if (!TemplateType) {
>> -    return TypeNode->getAsCXXRecordDecl();
>> -  }
>> -  if (TemplateType->getTemplateName().isDependent())
>> -    // Dependent template specializations will be matched when the
>> -    // template is instantiated.
>> -    return nullptr;
>> -
>> -  // For template specialization types which are specializing a template
>> -  // declaration which is an explicit or partial specialization of
>> another
>> -  // template declaration, getAsCXXRecordDecl() returns the corresponding
>> -  // ClassTemplateSpecializationDecl.
>> -  //
>> -  // For template specialization types which are specializing a template
>> -  // declaration which is neither an explicit nor partial specialization
>> of
>> -  // another template declaration, getAsCXXRecordDecl() returns NULL and
>> -  // we get the CXXRecordDecl of the templated declaration.
>> -  CXXRecordDecl *SpecializationDecl = TemplateType->getAsCXXRecordDecl();
>> -  if (SpecializationDecl) {
>> -    return SpecializationDecl;
>> -  }
>> -  NamedDecl *Templated =
>> -
>> TemplateType->getTemplateName().getAsTemplateDecl()->getTemplatedDecl();
>> -  if (CXXRecordDecl *TemplatedRecord =
>> dyn_cast<CXXRecordDecl>(Templated)) {
>> -    return TemplatedRecord;
>> -  }
>> -  // Now it can still be that we have an alias template.
>> -  TypeAliasDecl *AliasDecl = dyn_cast<TypeAliasDecl>(Templated);
>> -  assert(AliasDecl);
>> -  return getAsCXXRecordDecl(AliasDecl->getUnderlyingType().getTypePtr());
>> +static CXXRecordDecl *
>> +getAsCXXRecordDeclOrPrimaryTemplate(const Type *TypeNode) {
>> +  if (auto *RD = TypeNode->getAsCXXRecordDecl())
>> +    return RD;
>> +
>> +  // Find the innermost TemplateSpecializationType that isn't an alias
>> template.
>> +  auto *TemplateType = TypeNode->getAs<TemplateSpecializationType>();
>> +  while (TemplateType && TemplateType->isTypeAlias())
>> +    TemplateType =
>> +
>> TemplateType->getAliasedType()->getAs<TemplateSpecializationType>();
>> +
>> +  // If this is the name of a (dependent) template specialization, use
>> the
>> +  // definition of the template, even though it might be specialized
>> later.
>> +  if (TemplateType)
>> +    if (auto *ClassTemplate = dyn_cast_or_null<ClassTemplateDecl>(
>> +          TemplateType->getTemplateName().getAsTemplateDecl()))
>> +      return ClassTemplate->getTemplatedDecl();
>> +
>> +  return nullptr;
>>  }
>>
>>  // Returns true if the given class is directly or indirectly derived
>> @@ -800,7 +779,10 @@ bool MatchASTVisitor::classIsDerivedFrom
>>      if (typeHasMatchingAlias(TypeNode, Base, Builder))
>>        return true;
>>
>> -    CXXRecordDecl *ClassDecl = getAsCXXRecordDecl(TypeNode);
>> +    // FIXME: Going to the primary template here isn't really correct,
>> but
>> +    // unfortunately we accept a Decl matcher for the base class not a
>> Type
>> +    // matcher, so it's the best thing we can do with our current
>
>
> Why not? Specifically, it seems to match what the user wants in most
> cases. Now, I'm all for also exposing a more complex interface to the user,
> where they can / have to control exactly where to dive into the primary
> templates, but experience shows that this is really hard for anybody who's
> not a C++ expert to understand, and the current behavior here matches what
> people expect when they want to match classes in the inheritance hierarchy.
>

Read up on the other thread, and understand your concern now.
The problem in matchers is:
a) we want to be able to match primary templates, even if they're not
instantiated
b) the most common pattern to match on is names

In
template<typename T> class X : public Y<T> {}

I think it would be very surprising if we didn't match that X is derived
from a class called Y.


>
>
>> interface.
>> +    CXXRecordDecl *ClassDecl =
>> getAsCXXRecordDeclOrPrimaryTemplate(TypeNode);
>>      if (!ClassDecl)
>>        continue;
>>      if (ClassDecl == Declaration) {
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160210/ae7057d2/attachment-0001.html>