[cfe-commits] r85500 - in /cfe/trunk: include/clang/AST/ include/clang/Frontend/ lib/AST/ lib/Frontend/ lib/Sema/

[John McCall]

Douglas Gregor wrote:
> On Oct 29, 2009, at 1:12 AM, John McCall wrote:
>>
>> +/// Location information for a TemplateArgument.
>> +struct TemplateArgumentLocInfo {
>> +private:
>> + void *Union;
>
> Since this is storing either an Expr* or a DeclaratorInfo*, why not 
> use PointerUnion<Expr*, DeclaratorInfo*>? NULL can still represent the 
> empty state. Then, you won't have to do the #ifndef NDEBUG dance with 
> the Kind field.

It's the same idea as TypeLoc — the knowledge of how to interpret the 
information is externally derived, in this case from the template 
argument kind. There's no reason to impose the extra cost, because a 
type argument can only have a DeclaratorInfo, etc.

That said, I could certainly use an actual union.

>> +/// Location wrapper for a TemplateArgument. TemplateArgument is to
>> +/// TemplateArgumentLoc as Type is to TypeLoc.
>> +class TemplateArgumentLoc {
>> + TemplateArgument Argument;
>> + TemplateArgumentLocInfo LocInfo;
>> +
>> + friend class TemplateSpecializationTypeLoc;
>> + TemplateArgumentLoc(const TemplateArgument &Argument,
>> + TemplateArgumentLocInfo Opaque)
>> + : Argument(Argument), LocInfo(Opaque) {
>> + }
>> +
>> +public:
>> + TemplateArgumentLoc() {}
>> +
>> + TemplateArgumentLoc(const TemplateArgument &Argument, 
>> DeclaratorInfo *DInfo)
>> + : Argument(Argument), LocInfo(DInfo) {
>> + assert(Argument.getKind() == TemplateArgument::Type);
>> + }
>> +
>> + TemplateArgumentLoc(const TemplateArgument &Argument, Expr *E)
>> + : Argument(Argument), LocInfo(E) {
>> + assert(Argument.getKind() == TemplateArgument::Expression);
>> + }
>> +
>> + /// This is a temporary measure.
>> + TemplateArgumentLoc(const TemplateArgument &Argument)
>> + : Argument(Argument), LocInfo() {
>> + assert(Argument.getKind() != TemplateArgument::Expression &&
>> + Argument.getKind() != TemplateArgument::Type);
>> + }
>
> Is this temporary measure still needed, now that r85501 has gotten rid 
> of the K_None TemplateArgumentLocInfos?

It is, because we sometimes still transform non-canonical forms that 
don't have location information attached. But I'll try to isolate it more.

Later: Well, I've removed it, and there's only one remaining analogous 
call to TemplateArgumentLoc(Arg, TemplateArgumentLocInfo()), and that's 
in InventTemplateArgument.

>> + /// \brief - Fetches the start location of the argument, if possible.
>> + SourceLocation getLocation() const;
>
> It probably also makes sense to have a
>
> SourceRange getSourceRange() const;
>
> so that we can easily identify the whole template argument range.

Good idea. Added with uses as suggested.

>> @@ -1530,7 +1548,8 @@
>> if (E.isInvalid())
>> return true;
>>
>> - Arg = TemplateArgument(E.takeAs<Expr>());
>> + Expr *Ex = E.takeAs<Expr>();
>> + Arg = TemplateArgumentLoc(TemplateArgument(Ex), Ex);
>
> Ex->Retain() when building the template argument?

Done.

>> case TemplateArgument::Declaration: {
>> + // FIXME: we should never have to transform one of these.
>> DeclarationName Name;
>> if (NamedDecl *ND = dyn_cast<NamedDecl>(Arg.getAsDecl()))
>> Name = ND->getDeclName();
>> - TemporaryBase Rebase(*this, Arg.getLocation(), Name);
>> + TemporaryBase Rebase(*this, SourceLocation(), Name);
>> Decl *D = getDerived().TransformDecl(Arg.getAsDecl());
>> - if (!D)
>> - return TemplateArgument();
>> - return TemplateArgument(Arg.getLocation(), D);
>> + if (!D) return true;
>> +
>> + Output = TemplateArgumentLoc(TemplateArgument(D));
>> + return false;
>> }
>
> Is this FIXME correct? If we've already resolved a template argument 
> down to a specific declaration (which may be dependent but have known 
> type/kind), won't we end up just transforming that declaration?

Well, I've been treating declarations as a non-canonical form, because 
we only build arguments of declaration kind during argument conversion. 
Ideally, we would be able to assert during transformation that the 
template argument kind is either Expression or Type. That said, it's 
plausible that we'll need Declaration on that list for template template 
arguments, so I'll go ahead and try to preserve source information for 
declarations (as Expr*, for now).

>> +template<typename Derived>
>> +QualType TreeTransform<Derived>::TransformTemplateSpecializationType(
>> + const TemplateSpecializationType *TST,
>> + QualType ObjectType) {
>> + // FIXME: this entire method is a temporary workaround; callers
>> + // should be rewritten to provide real type locs.
>>
>> - return Result;
>> + // Fake up a TemplateSpecializationTypeLoc.
>> + TypeLocBuilder TLB;
>> + TemplateSpecializationTypeLoc TL
>> + = TLB.push<TemplateSpecializationTypeLoc>(QualType(TST, 0));
>> +
>> + TL.setTemplateNameLoc(getDerived().getBaseLocation());
>> + TL.setLAngleLoc(SourceLocation());
>> + TL.setRAngleLoc(SourceLocation());
>
> Could we use getDerived().getBaseLocation() here, so that we don't 
> lose all source-location information?

On the langle and rangle locations? Sure.

John.