SourceLocation of CXXConstructExpr nodes

Clang Frontend
1

Hello.

I'm trying to fix my patch [1] to make it pass all the tests.
Currently, Index/recursive-cxx-member-calls.cpp fails with my patch
because of the following issue.

Consider this code:

class Foo
{
public:
  Foo(const char *);
};

Foo func()
{
   const char *c;
   return Foo(c);
}

The AST currently produced for the return statement is:
(ReturnStmt <line:10:4, col:16>
    (CXXConstructExpr <col:11, col:16> 'class Foo''void (const class Foo &) throw()' elidable
      (MaterializeTemporaryExpr <col:11, col:16> 'const class Foo' lvalue
        (ImplicitCastExpr <col:11, col:16> 'const class Foo' <NoOp>
          (CXXFunctionalCastExpr <col:11, col:16> 'class Foo' functional cast to class Foo
            (CXXConstructExpr <col:11, col:15> 'class Foo''void (const char *)'
              (ImplicitCastExpr <col:15> 'const char *' <LValueToRValue>
                (DeclRefExpr <col:15> 'const char *' lvalue Var 0x7f92eb03afb0 'c' 'const char *')))))))))

After my patch, the CXXFunctionalCastExpr node becomes:
(CXXFunctionalCastExpr <col:11, col:16> 'class Foo' functional cast to class Foo <ConstructorConversion>
  (CXXConstructExpr <col:11, col:16> 'class Foo''void (const char *)'
    (ImplicitCastExpr <col:15> 'const char *' <LValueToRValue>
      (DeclRefExpr <col:15> 'const char *' lvalue Var 0x7fe0eb858640 'c' 'const char *')))))))))

You see that now the inner CXXConstructExpr source range ends at column 16, while before it used
to end at column 15.
Is there any reason why the CXXConstructExpr range should end one character before the CXXFunctionalCastExpr,
when they in fact refer to the same piece of code?
The c-index test fails because of this change, but I think the new behaviour is correct. If so, I'll update the test case.
Do you agree with me?

Thank you,
Nicola

2

I agree.

John.

3

Ok!
And then there's another similar issue, that cause the fail
of Index/get-cursor.cpp

Consider this code:
int func() {
  double value;
  return (int)value;
}

Currently, this is the AST of the return statement (without any patch from me):
(ReturnStmt <line:3:3, col:15>
    (CStyleCastExpr <col:10, col:15> 'int' <NoOp>
      (ImplicitCastExpr <col:15> 'int' <FloatingToIntegral>
        (ImplicitCastExpr <col:15> 'double' <LValueToRValue>
          (DeclRefExpr <col:15> 'double' lvalue Var 'value' 'double'))))))

Why does the source range of the C-style cast (and consequently the
return statement) end at the beginning of the casted expression (column 15)?
IMHO, it should end at column 19, where the "value" token ends.
If so, tests like get-cursor.cpp must be updated and they would currently fail.

This creates a problem with my patch, with a piece of code like this:
struct Foo {
  Foo(int);
};

Foo func() {
  int value;
  return (Foo)value;
}

Currently the AST for the return statement is:
(ReturnStmt <line:7:3, col:15>
    (CXXConstructExpr <col:10, col:15> 'struct Foo''void (const struct Foo &) throw()' elidable
      (MaterializeTemporaryExpr <col:10, col:15> 'const struct Foo' lvalue
        (ImplicitCastExpr <col:10, col:15> 'const struct Foo' <NoOp>
          (CStyleCastExpr <col:10, col:15> 'struct Foo' <ConstructorConversion>
            (CXXConstructExpr <col:10, col:15> 'struct Foo''void (int)'
              (ImplicitCastExpr <col:15> 'int' <LValueToRValue>
                (DeclRefExpr <col:15> 'int' lvalue Var 'value' 'int')))))))))

With my patch, it is:
(ReturnStmt <line:7:3, col:14>
    (CXXConstructExpr <col:10, col:14> 'struct Foo''void (const struct Foo &) throw()' elidable
      (MaterializeTemporaryExpr <col:10, col:14> 'const struct Foo' lvalue
        (ImplicitCastExpr <col:10, col:14> 'const struct Foo' <NoOp>
          (CStyleCastExpr <col:10, col:14> 'struct Foo' <ConstructorConversion>
            (CXXConstructExpr <col:10, col:14> 'struct Foo''void (int)'
              (ImplicitCastExpr <col:15> 'int' <LValueToRValue>
                (DeclRefExpr <col:15> 'int' lvalue Var 'value' 'int')))))))))

For some reason, my patch makes the CXXConstructExpr end one character before. The point,
however, is the same as before. The return statement and all the expression nodes should end
at the _end_ of the DeclRefExpr, in this case column 19, rather than the begin.
This one-character difference however make the (already wrong) get-cursor.cpp test fail with my patch.

At this point, since this issue goes beyond the scope of my patch,
if you agree with me that both behaviors are incorrect anyway, I'd update the test to make it pass anyway or
write it right and mark it as an expected failure, and file a bug for this issue.
In the meantime, it means my patch by itself is correct (in the sense that it doesn't introduce _new_ bugs)
and I can commit it.

Is it ok?

Nicola