I don't think this is fully what I am trying to fix with this patch. This patch addresses the inconsistency that the CXXConstructExpr contains the correct ParenRange for Stmts like:<div><br></div><div>X value(7);</div>
<div><br></div><div>But an incorrect range if the constructor is called like a function, e.g.:</div><div><br></div><div>X(7);</div><div><br></div><div>As for printing, I don't know whether the SourceRange of the CXXConstructExpr should include the parentheses. However, for refactoring, it is definitely highly beneficial to know it (and in fact I think we'll have to go and add it to other nodes as well). So, as CXXConstructExpr already contains a member for that, we should definitely populate it as best we can.</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 10, 2012 at 11:58 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div class="h5">On Mon, Jul 9, 2012 at 6:24 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I have noticed that the CXXConstructExpr is not always created with the correct parenthesis range. For example:<div><br></div><div><div>struct X { X(int x) {} };</div><div>void f() {</div><div> X value(7); // CXXConstructExpr has correct range (5, 12)</div>
<div> X(7); // CXXConstructExpr has incorrect range (3, 5), does not include ")"</div><div>}</div></div><div><br></div><div>Although I am not 100% sure this is a bug and not a feature, the attached patch fixes it. Please take a look.</div>
</blockquote><div><br></div></div></div><div>Here's the source ranges we currently give to the CXXConstructExpr:</div><div><br></div><div><font face="courier new, monospace">X value;</font></div><div><font face="courier new, monospace"> ^~~~~</font></div>
<div><span style="font-family:'courier new',monospace">X value(7);</span></div><div><font face="courier new, monospace"> ^~~~~~~~</font></div><div><font face="courier new, monospace">X value{7}; -ast-print produces "X value7;"!</font></div>
<div><font face="courier new, monospace"> ^~~~~~~</font></div><div><span style="font-family:'courier new',monospace">X value = 7;</span></div><div><span style="font-family:'courier new',monospace"> ^~~~~~~~~ (for 'value' copy constructor call)</span></div>
<div><font face="courier new, monospace"> ^ (for X(7) temporary constructor call)</font></div><div><font face="courier new, monospace">X value = {7}; -ast-print produces "X value = 7;"!</font></div><div>
<font face="courier new, monospace"> ^~</font></div><div><font face="courier new, monospace">X value = X(7);</font></div><div><font face="courier new, monospace"> ^~~~ (for 'value' copy constructor call)</font></div>
<div><font face="courier new, monospace"> ^~~ (for 'X(7)' temporary constructor call)</font></div><div><font face="courier new, monospace">X value = X{7}; -ast-print produces "X value = X(7);"!</font></div>
<div><font face="courier new, monospace"> ^~~~ (for 'value' copy constructor call)</font></div><div><font face="courier new, monospace"> ^ end loc invalid! (for 'X{7}' CXXTemporaryObjectExpr)</font></div>
<div><font face="arial, helvetica, sans-serif"><br></font></div><div><font face="arial, helvetica, sans-serif">For non-CXXConstructExpr initializations, here are the source ranges for the initializers:</font></div><div><font face="arial, helvetica, sans-serif"><br>
</font></div><div><span style="font-family:'courier new',monospace">int value = 7;</span></div><div><span style="font-family:'courier new',monospace"> ^</span></div><div><span style="font-family:'courier new',monospace">int value(7);</span></div>
<div><span style="font-family:'courier new',monospace"> ^</span></div><div><font face="courier new, monospace">int value{7};</font></div><div><font face="courier new, monospace"> ^~~</font></div>
<div><font face="courier new, monospace">int value = int(7);</font></div><div><font face="courier new, monospace"> ^~~~~~</font></div><div><font face="courier new, monospace">int value = int{7}; // -ast-print prints as int({ 7 })"!</font></div>
<div><font face="courier new, monospace"> ^ end loc invalid!</font></div><div><font face="courier new, monospace"> ^~~ (for InitListExpr within the CXXFunctionalCastExpr)</font></div><div><br></div>
<div><font face="arial, helvetica, sans-serif">I don't like changing the source ranges here without a clear picture of what is "right". To that end, m</font>y proposal is that the set of tokens in the source range for a node should match the set of tokens printed out by the AST printer for that node; that also has the advantage that we could easily write tests which cover both the AST printer and source ranges, without having to annotate where anything is supposed to start and end.</div>
<div><br></div><div>For a CXXConstructExpr, that would mean that enclosing parens are not part of the source range (but that enclosing braces are). That inconsistency seems unfortunate, but it is much more consistent than our current results.</div>
<div><br></div><div>(Note that we don't actually print the braces as part of a CXXConstructExpr yet -- that's because the StmtPrinter can't tell that braced initialization was used, since we fail to set the IsListInitialization flag when creating a CXXConstructExpr -- see the FIXME in BuildCXXConstructExpr in SemaDeclCXX for that.)</div>
</div>
</blockquote></div><br></div>