<br><br><div class="gmail_quote">On Mon, Oct 22, 2012 at 2:37 PM, Matthew Curtis <span dir="ltr"><<a href="mailto:mcurtis@codeaurora.org" target="_blank">mcurtis@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div class="im">
<div>On 10/8/2012 8:55 PM, Richard Trieu
wrote:<br>
</div>
</div><blockquote type="cite"><div class="im">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.
<br>
<fieldset></fieldset>
<br>
</div><pre>_______________________________________________
cfe-commits mailing list
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a>
</pre>
</blockquote>
<br>
A couple of minor things related to comments:<br>
<tt><br>
</tt>
<blockquote type="cite"><tt>@@ -566,6 +581,12 @@</tt><tt><br>
</tt><tt> (FlatTree[ReadNode].FromTD ||
FlatTree[ReadNode].ToTD);</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt> </tt><tt><br>
</tt><tt>+ /// NodeIsAPSInt - Returns true if the </tt><tt><b>arugments</b></tt><tt>
are stored in APSInt's.</tt><tt><br>
</tt></blockquote>
<tt> </tt><b><tt>^</tt></b><b><tt>
misspelling</tt></b><tt><br>
</tt>
<blockquote type="cite"><tt>+ bool NodeIsAPSInt() {</tt><tt><br>
</tt><tt>+ return FlatTree[ReadNode].IsValidFromInt ||</tt><tt><br>
</tt><tt>+ FlatTree[ReadNode].IsValidToInt;</tt><tt><br>
</tt><tt>+ }</tt><tt><br>
</tt><tt>+</tt><tt><br>
</tt></blockquote>
<tt><br>
</tt>
<blockquote type="cite"><tt> // Handle Expressions</tt><tt><br>
</tt></blockquote>
<tt> <b>^ should update comment now that we handle
Integrals</b></tt><tt><br>
</tt>
<blockquote type="cite"><tt> if (NonTypeTemplateParmDecl
*DefaultNTTPD =</tt><tt><br>
</tt><tt>
dyn_cast<NonTypeTemplateParmDecl>(ParamND)) {</tt><tt><br>
</tt><tt> Expr *FromExpr, *ToExpr;</tt><tt><br>
</tt><tt> llvm::APSInt FromInt, ToInt;</tt><tt><br>
</tt><tt> bool HasFromInt = !FromIter.isEnd() &&</tt><tt><br>
</tt><tt> FromIter->getKind() ==
TemplateArgument::Integral;</tt><tt><br>
</tt><tt> bool HasToInt = !ToIter.isEnd() &&</tt><tt><br>
</tt><tt> ToIter->getKind() ==
TemplateArgument::Integral;</tt><br>
</blockquote>
<br>
<br>
And an observation:<br>
<br>
The patch favors converting Exprs to Ints which may lose information
that might be helpful to the user. For example, compiling this:<br>
<blockquote><tt>template <int i> struct A { };</tt><br>
<tt>A<1+6> a1;</tt><br>
<tt>A<5+3> a2;</tt><br>
<tt>void f() { a1 = a2; }</tt><br>
</blockquote>
produces:<br>
<blockquote><tt>error: no viable overloaded '='</tt><tt><br>
</tt><tt>void f() { a1 = a2; }</tt><tt><br>
</tt><tt> ~~ ^ ~~</tt><tt><br>
</tt><tt>note: candidate function (the implicit copy assignment
operator) not viable: no known conversion from <b>'A<8>'
to 'A<7>'</b> for 1st argument</tt><tt><br>
</tt><tt>template <int i> struct A { };</tt><tt><br>
</tt><tt> ^</tt><tt><br>
</tt><tt>1 error generated.</tt><tt><br>
</tt></blockquote>
<br>
If we preserve Exprs in, we get:<br>
<br>
<blockquote><tt>error: no viable overloaded '='</tt><br>
<tt>void f() { a1 = a2; }</tt><br>
<tt> ~~ ^ ~~</tt><br>
<tt>note: candidate function (the implicit copy assignment
operator) not viable: no known conversion from <b>'A<5 +
3>' to 'A<7>'</b> for 1st argument</tt><br>
<tt>template <int i> struct A { };</tt><br>
<tt> ^</tt><br>
<tt>1 error generated.</tt><br>
</blockquote>
<br>
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.<br></div></blockquote><div><br></div><div>It doesn't seem as useful to have the whole expression on one side when the other side already has 1+6 changed to 7. This also allows for the assumption that if the two templates have an argument, they are of the same type.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
<br>
Otherwise LGTM (though I'm perhaps too much of a newbie here for my
approval to carry much weight).<br>
<br>
Cheers,<br>
Matthew Curtis. <br><span class="HOEnZb"><font color="#888888">
<br>
<br>
<pre cols="72">--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation</pre>
</font></span></div>
</blockquote></div><br>