Page MenuHomePhabricator

Make sizeof expression context partially evaluated
ClosedPublic

Authored by pmatos on Nov 18 2016, 1:50 AM.

Details

Reviewers
pmatos
efriedma
Summary

Ensure sizeof expression context is partially evaluated so that potential typeof operators inside are evaluated if necessary.

Fixes PR31042.

Diff Detail

Event Timeline

pmatos updated this revision to Diff 78487.Nov 18 2016, 1:50 AM
pmatos retitled this revision from to Make sizeof expression context partially evaluated.
pmatos updated this object.
pmatos added a reviewer: efriedma.
pmatos added a subscriber: cfe-commits.
EricWF added a subscriber: EricWF.Nov 18 2016, 4:42 AM

This isn't correct. For example this change breaks:

struct T { int m; };
int x = sizeof(T::m);

This isn't correct. For example this change breaks:

struct T { int m; };
int x = sizeof(T::m);

But that is not valid in C afaik and in C++ I get:

error: invalid use of non-static data member 'm'
  int x = sizeof(T::m);
                 ~~~^

Can you post a full reproducible example of what the change breaks?

But that is not valid in C afaik and in C++ I get:

error: invalid use of non-static data member 'm'
  int x = sizeof(T::m);
                 ~~~^

Can you post a full reproducible example of what the change breaks?

That is a full reproducible example because it's valid C++. Clang currently compiles it.

But that is not valid in C afaik and in C++ I get:

error: invalid use of non-static data member 'm'
  int x = sizeof(T::m);
                 ~~~^

Can you post a full reproducible example of what the change breaks?

That is a full reproducible example because it's valid C++. Clang currently compiles it.

But what was in the link was:

typedef struct { int m; } T;
int x = sizeof(T);
int main() {}

This is different from your initial snippet and compiles fine with my patch.

EricWF added a comment.EditedNov 18 2016, 10:22 AM

This is different from your initial snippet and compiles fine with my patch.

You're confused. Both examples compile fine w/o your patch and are rejected with it. I just double checked.

In case your wondering *why* this happens

But that is not valid in C afaik and in C++ I get:

error: invalid use of non-static data member 'm'
  int x = sizeof(T::m);
                 ~~~^

Can you post a full reproducible example of what the change breaks?

That is a full reproducible example because it's valid C++. Clang currently compiles it.

But what was in the link was:

typedef struct { int m; } T;
int x = sizeof(T);
int main() {}

This is different from your initial snippet and compiles fine with my patch.

Sorry your right. That should still say sizeof(T::m). My mistake.

Apologies if I am being shallow and wasting your time but sizeof(T::m) doesn't compile at the moment with clang trunk. Using the same service you used before.

OK, with a lot of help from @eli.friedman I have now a fix. Shall I reuse this review by submitting a new diff or open a new one?

efriedma edited edge metadata.Dec 9 2016, 8:08 AM

I would say make a new review thread since none of the discussion here is going to be helpful for understanding the new patch... but it really doesn't matter.

pmatos accepted this revision.Dec 15 2016, 1:32 AM
pmatos added a reviewer: pmatos.

Abandoning this revision.

This revision is now accepted and ready to land.Dec 15 2016, 1:32 AM
pmatos closed this revision.Dec 15 2016, 1:34 AM

This is not supposed to be marked as accepted.