Page MenuHomePhabricator

Add overload of TransformToPotentiallyEvaluated for TypeSourceInfo
Needs ReviewPublic

Authored by pmatos on Dec 15 2016, 1:30 AM.

Details

Reviewers
efriedma
Summary

Adds overload of ransformToPotentiallyEvaluated for TypeSourceInfo to properly deal with VLAs in nested calls of sizeof and typeof. Fixes #31042.

Diff Detail

Event Timeline

pmatos updated this revision to Diff 81547.Dec 15 2016, 1:30 AM
pmatos retitled this revision from to Add overload of TransformToPotentiallyEvaluated for TypeSourceInfo.
pmatos updated this object.
pmatos added a reviewer: efriedma.
pmatos added a subscriber: cfe-commits.

@efriedma Here's the new patch, thanks for your help getting here.

Can you add a test case?

efriedma requested changes to this revision.Jan 9 2017, 12:46 PM
efriedma edited edge metadata.

Missing regression test in test/SemaCXX/.

This revision now requires changes to proceed.Jan 9 2017, 12:46 PM

Apologies for the delay over the holiday season, I will look into this later on after office hours.

Has this been fixed upstream already?

Ah no, my mistake. I had the patch applied when I ran the test.

pmatos updated this revision to Diff 85328.EditedJan 22 2017, 11:32 PM
pmatos edited edge metadata.

Here's an updated patch including the test. Hope this is now ok for submission. Please accept my apologies with regards to the delay in submitting this.

efriedma added inline comments.Jan 23 2017, 12:58 PM
lib/Sema/SemaExpr.cpp
4027

Is the isUnevaluatedContext() check here necessary?

test/SemaCXX/pr31042.cpp
2

-emit-obj ?

4

Is this declaration necessary somehow?

pmatos marked 3 inline comments as done.Mar 14 2017, 1:54 AM

Thanks for the comments.

pmatos added inline comments.Mar 14 2017, 1:55 AM
lib/Sema/SemaExpr.cpp
4027

Not really. Reran tests to ensure removing it doesn't break anything.

test/SemaCXX/pr31042.cpp
2

Not needed. Left over from copying another test.

4

Same. Not needed. Left over from copying another test.

pmatos updated this revision to Diff 91684.Mar 14 2017, 1:58 AM

@efriedma I have uploaded a new patch taking your comments into consideration and rebased on most recent clang sources.

pmatos updated this revision to Diff 91685.Mar 14 2017, 2:00 AM

Added missing testcase to previous patch.

efriedma added inline comments.Mar 15 2017, 11:43 AM
test/SemaCXX/pr31042.cpp
2

Oh, this testcase doesn't actually crash on trunk without at least -emit-llvm because semantic analysis doesn't actually verify the used bit. :( Better to include that, I think.

pmatos updated this revision to Diff 91980.Mar 16 2017, 2:51 AM
pmatos marked an inline comment as done.
efriedma added inline comments.Mar 16 2017, 10:47 AM
test/SemaCXX/pr31042.cpp
1

You need to use "-o -" or something like that to avoid generating a file pr31042.ll. Also, a comment explaining why this test is using -emit-llvm would be nice.