[cfe-dev] r131701 and test/CodeGenObjC/debug-property-synth.m

[Adrian Prantl]

On Feb 10, 2014, at 23:15, David Blaikie <dblaikie at gmail.com> wrote:

> So I'm trying to do some stuff that's affecting .loc directives and came across a couple of test cases in Clang that, as was the fashion at the time, test LLVM. 
> 
> One of them is CodeGenObjC/debug-property-synth.m - which tests a particular .loc directive. The corresponding change it's meant to test is r131701, which seems to actually contain two independent changes. http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjC.cpp?r1=131373&r2=131701&pathrev=131701&diff_format=h

Wonderfully documented testcase :-p

>From looking at the radar, this test is supposed to test the location of synthesized getters and setters. We should not remove the testcase, but we should definitely make it more specific.

It should be testing that the synthesized setter/getter function for
     1	// RUN: %clang_cc1 -triple %itanium_abi_triple -masm-verbose -S -g %s -o - | FileCheck %s
     2	// Radar 9468526
     3	@interface I {
     4	  int _p1;
     5	}
     6	@property int p1;
     7	@end
     8	
     9	@implementation I
    10	@synthesize p1 = _p1;
    11	@end
    12	
    13	int main() {
    14	  I *myi;
    15	  myi.p1 = 2;
    16	  return 0;
    17	}
    18	
    19	// FIXME: Make this test ir files.
    20	// CHECK:       .loc    2 6 0

p1 are associated with line number 6.

> 
> 1) is the change at line 142 (and most of the changes in this patch - to pass the parameter through) to pass StartLoc instead of OMD->getLocStart()
> 
> 2) is the change at 492 to use PID instead of PD
> 

What are the respective values for StartLoc versus OMD->getLocStart() for this example?

> The problems I have with upgrading this test is that (1) is untested by this test file (I would /really/ like to revert this functionality if it's not tested, because I hate having unjustified complexity in the codebase - but perhaps you know of a way to test this piece of the patch?)
> 
> And (2) has changed a lot since the code was written (139466 addressed the fixme on the line above 492 - and removed anything that looks remotely like it). But I assume/hope that (2) is actually what the test case originally (and still does? maybe?) test. 
> 
> Should I just remove the test case? Is there a logical way to update it to test what it was originally intended to test? (is that even still a valid thing to test after r139466 - I assume so?)

-- adrian