Re: Patch for Bug 30413, including test case

[Akira Hatanaka via cfe-commits]

Committed r297702.

> On Mar 13, 2017, at 10:02 AM, Lobron, David  wrote:
> 
> Yes, please, if you don't mind!  I'd like to commit both the path and the 
> unit test, if possible.
> 
> Thanks,
> 
> David
> 
>> On Mar 13, 2017, at 12:47 PM, Akira Hatanaka  wrote:
>> 
>> Do you need someone to commit this patch for you?
>> 
>>> On Mar 10, 2017, at 6:44 AM, Lobron, David  wrote:
>>> 
>>> Hi Akira,
>>> 
>>> Thank you very much!  Please let me know if I need to take any further 
>>> steps beyond this email to cfe-commits in order for the patch and the unit 
>>> test to be committed.
>>> 
>>> Thanks,
>>> 
>>> David
>>> 
 On Mar 9, 2017, at 4:46 PM, Akira Hatanaka  wrote:
 
 Hi David,
 
 The patch looks good to me.
 
> On Mar 9, 2017, at 1:01 PM, Lobron, David  wrote:
> 
> Hi Akira,
> 
>> My concern is that the patch changes the encoding of 
>> @encode(id) on Darwin, which I think isn’t what you are trying 
>> to fix. If you compile the following code with command “clang -cc1 
>> -triple x86_64-apple-macosx”, the type encoding changes after applying 
>> the patch.
>> 
>> const char *foo() {
>> return @encode(id);
>> }
>> 
>> It seems like you can fix your problem without affecting Darwin by 
>> passing an extra argument to getObjCEncodingForType, just like 
>> CGObjCCommonMac::GetMethodVarType does.
> 
> Ah, thanks- I understand now.  Yes, this change seems a lot safer, and I 
> verified that it passes my test.  I've attached my new patch file, and 
> I've also attached the test again.  Please let me know if this works for 
> you or if you think it needs any additional work.
> 
> --David
> 
> 
 
>>> 
>> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch for Bug 30413, including test case

[Lobron, David via cfe-commits]

Yes, please, if you don't mind!  I'd like to commit both the path and the unit 
test, if possible.

Thanks,

David

> On Mar 13, 2017, at 12:47 PM, Akira Hatanaka  wrote:
> 
> Do you need someone to commit this patch for you?
> 
>> On Mar 10, 2017, at 6:44 AM, Lobron, David  wrote:
>> 
>> Hi Akira,
>> 
>> Thank you very much!  Please let me know if I need to take any further steps 
>> beyond this email to cfe-commits in order for the patch and the unit test to 
>> be committed.
>> 
>> Thanks,
>> 
>> David
>> 
>>> On Mar 9, 2017, at 4:46 PM, Akira Hatanaka  wrote:
>>> 
>>> Hi David,
>>> 
>>> The patch looks good to me.
>>> 
 On Mar 9, 2017, at 1:01 PM, Lobron, David  wrote:
 
 Hi Akira,
 
> My concern is that the patch changes the encoding of 
> @encode(id) on Darwin, which I think isn’t what you are trying 
> to fix. If you compile the following code with command “clang -cc1 
> -triple x86_64-apple-macosx”, the type encoding changes after applying 
> the patch.
> 
> const char *foo() {
> return @encode(id);
> }
> 
> It seems like you can fix your problem without affecting Darwin by 
> passing an extra argument to getObjCEncodingForType, just like 
> CGObjCCommonMac::GetMethodVarType does.
 
 Ah, thanks- I understand now.  Yes, this change seems a lot safer, and I 
 verified that it passes my test.  I've attached my new patch file, and 
 I've also attached the test again.  Please let me know if this works for 
 you or if you think it needs any additional work.
 
 --David
 
 
>>> 
>> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch for Bug 30413, including test case

[Akira Hatanaka via cfe-commits]

Do you need someone to commit this patch for you?

> On Mar 10, 2017, at 6:44 AM, Lobron, David  wrote:
> 
> Hi Akira,
> 
> Thank you very much!  Please let me know if I need to take any further steps 
> beyond this email to cfe-commits in order for the patch and the unit test to 
> be committed.
> 
> Thanks,
> 
> David
> 
>> On Mar 9, 2017, at 4:46 PM, Akira Hatanaka  wrote:
>> 
>> Hi David,
>> 
>> The patch looks good to me.
>> 
>>> On Mar 9, 2017, at 1:01 PM, Lobron, David  wrote:
>>> 
>>> Hi Akira,
>>> 
 My concern is that the patch changes the encoding of @encode(id) 
 on Darwin, which I think isn’t what you are trying to fix. If you compile 
 the following code with command “clang -cc1 -triple x86_64-apple-macosx”, 
 the type encoding changes after applying the patch.
 
 const char *foo() {
 return @encode(id);
 }
 
 It seems like you can fix your problem without affecting Darwin by passing 
 an extra argument to getObjCEncodingForType, just like 
 CGObjCCommonMac::GetMethodVarType does.
>>> 
>>> Ah, thanks- I understand now.  Yes, this change seems a lot safer, and I 
>>> verified that it passes my test.  I've attached my new patch file, and I've 
>>> also attached the test again.  Please let me know if this works for you or 
>>> if you think it needs any additional work.
>>> 
>>> --David
>>> 
>>> 
>> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch for Bug 30413, including test case

[Lobron, David via cfe-commits]

Hi Akira,

Thank you very much!  Please let me know if I need to take any further steps 
beyond this email to cfe-commits in order for the patch and the unit test to be 
committed.

Thanks,

David

> On Mar 9, 2017, at 4:46 PM, Akira Hatanaka  wrote:
> 
> Hi David,
> 
> The patch looks good to me.
> 
>> On Mar 9, 2017, at 1:01 PM, Lobron, David  wrote:
>> 
>> Hi Akira,
>> 
>>> My concern is that the patch changes the encoding of @encode(id) 
>>> on Darwin, which I think isn’t what you are trying to fix. If you compile 
>>> the following code with command “clang -cc1 -triple x86_64-apple-macosx”, 
>>> the type encoding changes after applying the patch.
>>> 
>>> const char *foo() {
>>> return @encode(id);
>>> }
>>> 
>>> It seems like you can fix your problem without affecting Darwin by passing 
>>> an extra argument to getObjCEncodingForType, just like 
>>> CGObjCCommonMac::GetMethodVarType does.
>> 
>> Ah, thanks- I understand now.  Yes, this change seems a lot safer, and I 
>> verified that it passes my test.  I've attached my new patch file, and I've 
>> also attached the test again.  Please let me know if this works for you or 
>> if you think it needs any additional work.
>> 
>> --David
>> 
>> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch for Bug 30413, including test case

[Akira Hatanaka via cfe-commits]

Hi David,

The patch looks good to me.

> On Mar 9, 2017, at 1:01 PM, Lobron, David  wrote:
> 
> Hi Akira,
> 
>> My concern is that the patch changes the encoding of @encode(id) 
>> on Darwin, which I think isn’t what you are trying to fix. If you compile 
>> the following code with command “clang -cc1 -triple x86_64-apple-macosx”, 
>> the type encoding changes after applying the patch.
>> 
>> const char *foo() {
>> return @encode(id);
>> }
>> 
>> It seems like you can fix your problem without affecting Darwin by passing 
>> an extra argument to getObjCEncodingForType, just like 
>> CGObjCCommonMac::GetMethodVarType does.
> 
> Ah, thanks- I understand now.  Yes, this change seems a lot safer, and I 
> verified that it passes my test.  I've attached my new patch file, and I've 
> also attached the test again.  Please let me know if this works for you or if 
> you think it needs any additional work.
> 
> --David
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch for Bug 30413, including test case

[Lobron, David via cfe-commits]

Hi Akira,

> My concern is that the patch changes the encoding of @encode(id) on 
> Darwin, which I think isn’t what you are trying to fix. If you compile the 
> following code with command “clang -cc1 -triple x86_64-apple-macosx”, the 
> type encoding changes after applying the patch.
> 
> const char *foo() {
>  return @encode(id);
> }
> 
> It seems like you can fix your problem without affecting Darwin by passing an 
> extra argument to getObjCEncodingForType, just like 
> CGObjCCommonMac::GetMethodVarType does.

Ah, thanks- I understand now.  Yes, this change seems a lot safer, and I 
verified that it passes my test.  I've attached my new patch file, and I've 
also attached the test again.  Please let me know if this works for you or if 
you think it needs any additional work.

--David



CGObjCGNU.cpp.patch
Description: CGObjCGNU.cpp.patch


ivar-type-encoding.m
Description: ivar-type-encoding.m
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch for Bug 30413, including test case

[Akira Hatanaka via cfe-commits]

> On Mar 6, 2017, at 10:17 AM, Lobron, David  wrote:
> 
> Hi Akira,
> 
> Pardon my slowness in addressing your question.
> 
>> This patch changes the encoding of an id conforming to a protocol, which I 
>> think was not intended: For example:
>> 
>> @encode(id)
>> 
>> Would passing IVD to the call to getObjCEncodingForType in 
>> CGObjCGNU::GenerateClass solve the problem?
> 
> Are you suggesting that passing IVD instead of just IVD->getType() will tell 
> getObjCEncodingForType whether the code being compiled is a protocol, as 
> opposed to an object?  I'm a little confused here, because IVD is an object 
> of type ObjCIvarDecl, which is supposed to represent an instance variable, 
> not a protocol.  Also, I don't see anything in the definition of ObjCIvarDecl 
> (in AST/DeclObjC.h) that would tell us whether it represents an instance 
> variable or a protocol.  Can you explain the case you are concerned about a 
> little more?
> 

My concern is that the patch changes the encoding of @encode(id) on 
Darwin, which I think isn’t what you are trying to fix. If you compile the 
following code with command “clang -cc1 -triple x86_64-apple-macosx”, the type 
encoding changes after applying the patch.

const char *foo() {
  return @encode(id);
}

It seems like you can fix your problem without affecting Darwin by passing an 
extra argument to getObjCEncodingForType, just like 
CGObjCCommonMac::GetMethodVarType does.

> If IVD can in fact tell us whether the code being compiled is an object, then 
> we can make the EncodeClassNames argument of getObjCEncodingForTypeImpl true 
> for ivars and false for protocols.
> 
> Thanks!
> 
> --David

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch for Bug 30413, including test case

[Lobron, David via cfe-commits]

Hi Akira,

Pardon my slowness in addressing your question.

> This patch changes the encoding of an id conforming to a protocol, which I 
> think was not intended: For example:
> 
> @encode(id)
> 
> Would passing IVD to the call to getObjCEncodingForType in 
> CGObjCGNU::GenerateClass solve the problem?

Are you suggesting that passing IVD instead of just IVD->getType() will tell 
getObjCEncodingForType whether the code being compiled is a protocol, as 
opposed to an object?  I'm a little confused here, because IVD is an object of 
type ObjCIvarDecl, which is supposed to represent an instance variable, not a 
protocol.  Also, I don't see anything in the definition of ObjCIvarDecl (in 
AST/DeclObjC.h) that would tell us whether it represents an instance variable 
or a protocol.  Can you explain the case you are concerned about a little more?

If IVD can in fact tell us whether the code being compiled is an object, then 
we can make the EncodeClassNames argument of getObjCEncodingForTypeImpl true 
for ivars and false for protocols.

Thanks!

--David
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch for Bug 30413, including test case

[Lobron, David via cfe-commits]

Hi Akira,

Pardon my slow reply here- I was traveling and just got back to work email.  I 
will check into this as soon as I can, and get back to you.

Thank you,

David

> On Feb 20, 2017, at 12:04 AM, Akira Hatanaka  wrote:
> 
> This patch changes the encoding of an id conforming to a protocol, which I 
> think was not intended: For example:
> 
> @encode(id)
> 
> Would passing IVD to the call to getObjCEncodingForType in 
> CGObjCGNU::GenerateClass solve the problem?
> 
>> On Feb 15, 2017, at 1:59 PM, Lobron, David via cfe-commits 
>>  wrote:
>> 
>> Hi All,
>> 
>> I am re-submitting my patch for Bug 30413, this time with a test case 
>> included as well (ivar-type-encoding.m).  The test case file should be added 
>> to clang/test/CodeGenObjC.  The test verifies that correct metadata is 
>> emitted by clang for an object-valued instance variable.  I've verified that 
>> the test passes when the patch has been applied to ASTContext.cpp, and fails 
>> otherwise.
>> 
>> Please let me know if this looks OK, or if any additional information is 
>> needed.
>> 
>> Thanks,
>> 
>> David
>> 
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch for Bug 30413, including test case

[Akira Hatanaka via cfe-commits]

This patch changes the encoding of an id conforming to a protocol, which I 
think was not intended: For example:

@encode(id)

Would passing IVD to the call to getObjCEncodingForType in 
CGObjCGNU::GenerateClass solve the problem?

> On Feb 15, 2017, at 1:59 PM, Lobron, David via cfe-commits 
>  wrote:
> 
> Hi All,
> 
> I am re-submitting my patch for Bug 30413, this time with a test case 
> included as well (ivar-type-encoding.m).  The test case file should be added 
> to clang/test/CodeGenObjC.  The test verifies that correct metadata is 
> emitted by clang for an object-valued instance variable.  I've verified that 
> the test passes when the patch has been applied to ASTContext.cpp, and fails 
> otherwise.
> 
> Please let me know if this looks OK, or if any additional information is 
> needed.
> 
> Thanks,
> 
> David
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Patch for Bug 30413, including test case

[Lobron, David via cfe-commits]

Hi All,

I am re-submitting my patch for Bug 30413, this time with a test case included 
as well (ivar-type-encoding.m).  The test case file should be added to 
clang/test/CodeGenObjC.  The test verifies that correct metadata is emitted by 
clang for an object-valued instance variable.  I've verified that the test 
passes when the patch has been applied to ASTContext.cpp, and fails otherwise.

Please let me know if this looks OK, or if any additional information is needed.

Thanks,

David



ivar-type-encoding.m
Description: ivar-type-encoding.m


PatchForBug30413.patch
Description: PatchForBug30413.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits