[cfe-commits] [Patch] Add handling of APSInt to Template Type Diffing - Fixes PR14015

[Matthew Curtis]

On 10/8/2012 8:55 PM, Richard Trieu wrote:
> An assumption during the creation of Template Type Diffing expected 
> that integral arguments would be available as Expr's (e.g. 
> IntegralLiteral).  However, it appears that in some cases, the 
> TemplateArgument gives an Expr, and in others, TemplateArgument only 
> gives an APSInt.  This patch allows Template Type Diffing to handle 
> APSInt by storing in inside the tree nodes, as well as other bits of 
> information needed.  Also, in cases where one argument is given as an 
> Expr and the other as an APSInt, evaluate the Expr and store the 
> result as an APSInt.
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

A couple of minor things related to comments:

> @@ -566,6 +581,12 @@
>               (FlatTree[ReadNode].FromTD || FlatTree[ReadNode].ToTD);
>      }
>
> +    /// NodeIsAPSInt - Returns true if the *arugments*are stored in 
> APSInt's.
*^**misspelling*
> +    bool NodeIsAPSInt() {
> +      return FlatTree[ReadNode].IsValidFromInt ||
> +             FlatTree[ReadNode].IsValidToInt;
> +    }
> +

>       // Handle Expressions
*^ should update comment now that we handle Integrals*
>       if (NonTypeTemplateParmDecl *DefaultNTTPD =
> dyn_cast<NonTypeTemplateParmDecl>(ParamND)) {
>         Expr *FromExpr, *ToExpr;
>         llvm::APSInt FromInt, ToInt;
>         bool HasFromInt = !FromIter.isEnd() &&
>                           FromIter->getKind() == 
> TemplateArgument::Integral;
>         bool HasToInt = !ToIter.isEnd() &&
>                         ToIter->getKind() == TemplateArgument::Integral;


And an observation:

The patch favors converting Exprs to Ints which may lose information 
that might be helpful to the user. For example, compiling this:

    template <int i> struct A { };
    A<1+6> a1;
    A<5+3> a2;
    void f() { a1 = a2; }

produces:

    error: no viable overloaded '='
    void f() { a1 = a2; }
                ~~ ^ ~~
    note: candidate function (the implicit copy assignment operator) not
    viable: no known conversion from *'A<8>' to 'A<7>'* for 1st argument
    template <int i> struct A { };
                             ^
    1 error generated.


If we preserve Exprs in, we get:

    error: no viable overloaded '='
    void f() { a1 = a2; }
                ~~ ^ ~~
    note: candidate function (the implicit copy assignment operator) not
    viable: no known conversion from *'A<5 + 3>' to 'A<7>'* for 1st argument
    template <int i> struct A { };
                             ^
    1 error generated.


Not sure how important this is, just something I noticed. In this simple 
example it's obvious that '8' is '5 + 3', but I could see a more 
complicated expression causing confusion.

Otherwise LGTM (though I'm perhaps too much of a newbie here for my 
approval to carry much weight).

Cheers,
Matthew Curtis.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121022/8fee3df9/attachment.html>