[cfe-commits] [Request for approval] Added basic source locations to Elaborated and DependentName types.

[John McCall]

On May 18, 2010, at 2:42 PM, Abramo Bagnara wrote:

> Il 14/05/2010 19:48, John McCall ha scritto:
>> 
>> On May 14, 2010, at 7:55 AM, Abramo Bagnara wrote:
>> 
>>> Il 13/05/2010 22:43, John McCall ha scritto:
>>>> On May 13, 2010, at 2:09 AM, Abramo Bagnara wrote:
>>>>> The attached patch improves support for locations inside Elaborated
>>>>> and DependentName types.
>>> 
>>> I've committed the fixed patch in r103770.
>> 
>> I notice that you didn't include the SourceRange for the NestedNameSpecifiers
>> like I asked.  Without this, your calculation of the source ranges for these
>> TypeLocs is incorrect, particularly because you're consistently assuming that
>> KeywordLoc is a real source location — it's meaningless for ETK_None,
>> which is possible for both ElaboratedType and DependentNameType.
>> 
>> Please fix this.
> 
> After some pain to fix some obscure details, we have revised the
> reverted patch to add NestedNameSpecifiers location ranges and to fix
> failures due to missing compilation of TemplateArgLocInfo.
> 
> Please review the attached patch.

Looks good!  I only have two complaints.

First, you missed PCH support for the qualifier range of ElaboratedType and DependentNameType.

Second, you have a couple large blocks like this (one in TreeTransform, one in SemaTemplate):

+  if (const ElaboratedType* ElabT = Result->getAs<ElaboratedType>()) {
+    QualType NamedT = ElabT->getNamedType();
+    if (isa<TypedefType>(NamedT)) {
+      TypedefTypeLoc NamedTLoc = TLB.push<TypedefTypeLoc>(NamedT);
+      NamedTLoc.setNameLoc(TL.getNameLoc());
+    }
+    else if (isa<RecordType>(NamedT)) {
+      RecordTypeLoc NamedTLoc = TLB.push<RecordTypeLoc>(NamedT);
+      NamedTLoc.setNameLoc(TL.getNameLoc());
+    }
+    else if (isa<EnumType>(NamedT)) {
+      EnumTypeLoc NamedTLoc = TLB.push<EnumTypeLoc>(NamedT);
+      NamedTLoc.setNameLoc(TL.getNameLoc());
+    }
+    else if (isa<TemplateSpecializationType>(NamedT)) {
+      TemplateSpecializationTypeLoc NamedTLoc
+        = TLB.push<TemplateSpecializationTypeLoc>(NamedT);
+      // FIXME: fill locations
+      NamedTLoc.initializeLocal(TL.getNameLoc());
+    }
+    else
+      llvm_unreachable("Unexpected type");
+    ElaboratedTypeLoc NewTL = TLB.push<ElaboratedTypeLoc>(Result);
+    NewTL.setKeywordLoc(TL.getKeywordLoc());
+    NewTL.setQualifierRange(TL.getQualifierRange());
+  }

Instead of calling out N different cases of single-token type names, it should be sufficient to write:

  if (const ElaboratedType *ElabT = Result->getAs<ElaboratedType()) {
    QualType NamedT = ElabT->getNamedType();
    if (isa<TemplateSpecializationType>(NamedT)) {
      ...
    } else {
      TLB.pushTypeSpec(NamedT).setNameLoc(TL.getNameLoc());
    }
    ElaboratedTypeLoc NewTL = TLB.push<ElaboratedTypeLoc>(Result);
    ...
  }

John.