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

[Richard Smith]

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/20120710/c74957dc/attachment.html>