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

Richard Smith richard at metafoo.co.uk
Tue Jul 10 15:33:08 PDT 2012


On Tue, Jul 10, 2012 at 3:07 PM, Daniel Jasper <djasper at google.com> wrote:

> 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);
>

Right. My point is we don't have rules for what a correct range would be,
and we need such rules before we can say whether your patch is right. My
proposed rule means that the source range is wrong in both cases, and
shouldn't include the parens (and that we should drop the ParenRange from
CXXConstructExpr entirely). The fix for that is completely different from
what you're proposing :-)

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.
>

That presumes that the parens are somehow logically part of the
CXXConstructExpr, which I think is really the relevant question here.
CXXConstructExpr is used in various cases where the parens are either part
of some other construct, or where there are no parens, so I don't think it
makes much sense to include the parens in the source range. I would imagine
that for refactoring, what's really desired is a consistent and rational
set of rules for what the source range of an expression means, which, as my
examples demonstrate, we're *really really far* from having.

Consider these two cases:

X value(7);
int value(7);

In the first case, the source range for the initializer covers 'value' and
the parentheses. For the second case, it covers only the '7'. The right fix
for the second case seems naturally to be that we should store the source
locations of the parentheses on the VarDecl. And that would remove any need
for storing them on the CXXConstructExpr in the first case.

Likewise in these expressions:

X(7)
int(7)

Here, the CXXFunctionalCastExpr already contains the source locations of
the parentheses. In the 'int' case, we don't store them anywhere else (and
nor do we need to). In the 'X' case, we include the 'X' and the '(' in the
source range of the CXXConstructExpr. That is inconsistent and unnecessary.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120710/d7215422/attachment.html>


More information about the cfe-commits mailing list