On Jul 10, 2012, at 10:23 PM, Richard Smith wrote: > On Tue, Jul 10, 2012 at 10:05 PM, Daniel Jasper <[email protected]> wrote: > On Wed, Jul 11, 2012 at 12:33 AM, Richard Smith <[email protected]> wrote: > On Tue, Jul 10, 2012 at 3:07 PM, Daniel Jasper <[email protected]> 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. > > Why do we not need the parenthesis for int(7)? Because there are no > diagnostics that can trigger on a range including them? > > Because they can be derived from the CXXFunctionalCastExpr (also, there's no > other AST node where they could reasonably go).
CXXConstructExpr generally doesn't imply a particular syntactic form. Instead of putting the parens range for a direct-initialization there, why don't we introduce a CXXDirectInitExpr expression instead? That way we'd still store the parens for scalars and conversion operators. John.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
