Page MenuHomePhabricator

test/SemaObjC: Remove cruft in unused getter test
ClosedPublic

Authored by modocache on Aug 14 2015, 7:42 PM.

Details

Summary

The tests that verify that accessing a property without using the result
emits a warning were needlessly complicated. Remove several layers of
abstraction to make the tests much simpler to read and reason about.

Diff Detail

Event Timeline

modocache updated this revision to Diff 32213.Aug 14 2015, 7:42 PM
modocache retitled this revision from to test/SemaObjC: Remove cruft in unused getter test.
modocache updated this object.
modocache added a reviewer: cfe-commits.
modocache added a subscriber: cfe-commits.

Why do you think it's a cruft? Seems it's a bit more verbose than it should be, but what is missing in your test is inheritance, which is important.

P.S. I think the code for the initial test was just extracted from a real project.

Why do you think it's a cruft? Seems it's a bit more verbose than it should be

The verbosity is the cruft, in my opinion. Why spend thirty lines of code to demonstrate behavior that could be demonstrated in just five?

but what is missing in your test is inheritance, which is important.

Is it, though? I haven't read the source code completely, but there doesn't seem to be anything in there written specifically to handle inheritence as deep as the test had it. I believe the relevant code is in Expr::isUnusedResultAWarning(), which has case statements for case ObjCMessageExprClass and case ObjCPropertyRefExprClass. The behavior doesn't change based on where those are defined, whether on a base class or, as in this change, on a protocol.

I think the code for the initial test was just extracted from a real project.

Yep, based on the header prefix XC I'm guessing it was something in Xcode. And while I appreciate the fact that it's a "real-world" example, I think it's valuable to reduce this sort of thing to the minimum amount of code necessary to demonstrate the behavior.

s/which is important/which is might be important'/
;)

IMHO, the purpose of the test is not just prove that functionality is working as expected, but also prevent from regression, I completely agree that this test can be cleaned up a bit, but I'd cover at least two cases: a property of a class itself and a property of a superclass.
Though, it's better to ask somebody with a proper understanding of internals :)

rjmccall edited edge metadata.Aug 19 2015, 11:39 AM

Sure, fine to me.

AlexDenisov accepted this revision.Aug 21 2015, 1:36 PM
AlexDenisov added a reviewer: AlexDenisov.

Committed, r245731.

This revision is now accepted and ready to land.Aug 21 2015, 1:36 PM
AlexDenisov closed this revision.Aug 21 2015, 1:36 PM