Broken sizeof(expr) location in SemaExpr.cpp

Hi,

Try `clang -cc1 -ast-dump' on the following case using the trunk
version, where both sizeof statements should start at column 3.

void foo(int i)
{
  sizeof(i);
  sizeof(int);
}

Clang outputs

  (UnaryExprOrTypeTraitExpr ... <line:3:9, col:11> 'unsigned long' sizeof
  ...
  (UnaryExprOrTypeTraitExpr ... <line:4:3, col:13> 'unsigned long'
sizeof 'int'))

Note that when sizeof is applied to an expression, sizeof(i), the
column number is 9, rather than 3; sizeof(int) is still good.

I looked through the recent commits. In rev. 132115, at
lib/Sema/SemaExpr.cpp:3265, the interface of
Sena::CreateUnaryExprOrTypeTraitExpr was changed --- it no longer
takes OpLoc as an argument when sizeof is applied to an expression.
At line 3290, when creating UnaryExprOrTypeTraitExpr, the code uses
E->getExprLoc() as the location of sizeof. This means that in the
example above, the OpLoc of sizeof(i) was actually that of i.

I noticed this change because it bit my Clang-based source rewriter.
Is this intentional or a bug? Thanks.

- xi

This does seem wrong. Since you've done so much of the
investigation on it, care to write up a patch? :slight_smile:

John.

I'll look at this tonight. It's my bug, I made the change, and I see
clearly the error. The weird part is that I tried to add an assertion
to catch just this error and it didn't fire.

Cool. Thanks a lot. I would suggest to add the two parameters OpLoc
and ArgRange back. This interface change also affects Sema.h and
TreeTransform.h.

BTW, the assertion you wanted might be "ArgEx->getExprLoc() == OpLoc" :wink:

Cool. Thanks a lot. I would suggest to add the two parameters OpLoc
and ArgRange back.

ArgRange shouldn't be needed. The entire argument is an expression, so
the expression should have the proper source range for the argument
tokens. The location of the operator itself was what we lost here.

This interface change also affects Sema.h and
TreeTransform.h.

Yep, I changed it before, so I'm aware.

BTW, the assertion you wanted might be "ArgEx->getExprLoc() == OpLoc" :wink:

I had exactly this assertion.... but I inverted the logic sadly. =[
Sorry for the problems, I wouldn't have removed OpLoc except that I
thought my assertion had proven its futility... doh!

All fixed in r132284. I've manually verified yours and several other
test cases, but let me know if something still isn't working.

Cool. It works for me. Everything looks great. Thanks!

- xi