[cfe-commits] Patch for review: Parenthesis range in CXXConstructExprs

[Daniel Jasper]

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:

X value(7);

But an incorrect range if the constructor is called like a function, e.g.:

X(7);

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.


On Tue, Jul 10, 2012 at 11:58 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Mon, Jul 9, 2012 at 6:24 AM, Daniel Jasper <djasper at google.com> wrote:
>
>> I have noticed that the CXXConstructExpr is not always created with the
>> correct parenthesis range. For example:
>>
>> struct X { X(int x) {} };
>> void f() {
>>   X value(7);  // CXXConstructExpr has correct range (5, 12)
>>   X(7);  // CXXConstructExpr has incorrect range (3, 5), does not include
>> ")"
>> }
>>
>> Although I am not 100% sure this is a bug and not a feature, the attached
>> patch fixes it. Please take a look.
>>
>
> Here's the source ranges we currently give to the CXXConstructExpr:
>
> X value;
>   ^~~~~
> X value(7);
>   ^~~~~~~~
> X value{7};  -ast-print produces "X value7;"!
>   ^~~~~~~
> X value = 7;
>   ^~~~~~~~~ (for 'value' copy constructor call)
>           ^ (for X(7) temporary constructor call)
> X value = {7};  -ast-print produces "X value = 7;"!
>           ^~
> X value = X(7);
>           ^~~~ (for 'value' copy constructor call)
>           ^~~ (for 'X(7)' temporary constructor call)
> X value = X{7};  -ast-print produces "X value = X(7);"!
>           ^~~~ (for 'value' copy constructor call)
>           ^ end loc invalid! (for 'X{7}' CXXTemporaryObjectExpr)
>
> For non-CXXConstructExpr initializations, here are the source ranges for
> the initializers:
>
> int value = 7;
>             ^
> int value(7);
>           ^
> int value{7};
>          ^~~
> int value = int(7);
>             ^~~~~~
> int value = int{7};  // -ast-print prints as int({ 7 })"!
>             ^ end loc invalid!
>                ^~~ (for InitListExpr within the CXXFunctionalCastExpr)
>
> I don't like changing the source ranges here without a clear picture of
> what is "right". To that end, my 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.
>
> 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.
>
> (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.)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120711/1260c28b/attachment.html>