You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by ChristopherRogers <gi...@git.apache.org> on 2016/01/13 04:53:42 UTC
[GitHub] thrift pull request: THRIFT-3545 Cocoa: Container type literals do...
GitHub user ChristopherRogers opened a pull request:
https://github.com/apache/thrift/pull/790
THRIFT-3545 Cocoa: Container type literals do not compile
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ChristopherRogers/thrift THRIFT-3545
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/790.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #790
----
commit 1600191b1847ed06e77ab7ca501ce3747ae66212
Author: Christopher Rogers <ch...@linecorp.com>
Date: 2016-01-13T03:52:41Z
THRIFT-3545 Cocoa: Container type literals do not compile
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3545 Cocoa: Container type literals do...
Posted by ChristopherRogers <gi...@git.apache.org>.
Github user ChristopherRogers commented on the pull request:
https://github.com/apache/thrift/pull/790#issuecomment-172571012
Ah, my apologies for the extra semicolons. It was missing semicolons in the default init methods where a collection type had a default value.
I believe this semicolon problem is due to discrepancies in how the caller of this method assumes code will be generated--with semicolons and line breaks or not. I think if the call sites were to be cleaned up the ugliness would go away. I believe I checked them but I might've overlooked them. I'll give it another look again later too.
The NSSet change should've been in another PR, I admit, but it was for performance reasons (since one of Thrift's main goals is performance). I believe that optimized ARC code cannot cancel out the autorelease that setWithArray makes because Foundation isn't using ARC itself (I think, but it's worth testing.) Even if it could I think it's still faster to not have to have it back out of autoreleasing. I considered changing it to the initWithObjects variant but it seemed a little too drastic and needs performance testing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3545 Cocoa: Container type literals do...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/790#issuecomment-172355763
Have these changes been tested with nested containers? I don't think they will work.
My apologies for being late to the party. I am only scanning my thrift email intermittently lately.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3545 Cocoa: Container type literals do...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/790#issuecomment-172365685
@ChristopherRogers Please check #797 and see if it produces working code for you as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3545 Cocoa: Container type literals do...
Posted by ChristopherRogers <gi...@git.apache.org>.
Github user ChristopherRogers commented on the pull request:
https://github.com/apache/thrift/pull/790#issuecomment-172575020
Also, I just remembered from an old version of Xcode that even the @[]/@{} syntax used autorelease (probably to easily codegen in both ARC and MRC) but that might've changed since then, and I might've been looking at a non-optimized build.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3545 Cocoa: Container type literals do...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/790#issuecomment-172362021
These do work with nested types so my hunch was wrong on that but with the current code (including your PR) it now has awkward extra semi-colons; so my munch was somewhat correct that they appear unnecessary
The NSSet missing a single brace was correct. I don't know why you switched it to the longer syntax when you could have just added a brace.
It seems to me the issue was only to do with set literals and could have been solved with a single character change (adding a brace). Is there some other place I am missing where semi-colons are not added? If so I cannot seem to find it.
Currently as generated...
```objc
NSDictionary<NSString *, NSString *> * tmp0 = @{@"new": @"world"};
MapConst = @{@"hello": tmp0};
;
NSSet<NSString *> * tmp1 = [[NSSet alloc] initWithArray:@[@"hello"]];
SetConst = [[NSSet alloc] initWithArray:@[tmp1]];
;
NSArray<NSString *> * tmp2 = @[@"hello"];
ArrayConst = @[tmp2];
;
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3545 Cocoa: Container type literals do...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/790#issuecomment-172357126
@ChristopherRogers Can you provide an example of what does not compile for you?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3545 Cocoa: Container type literals do...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/790
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3545 Cocoa: Container type literals do...
Posted by kdubb <gi...@git.apache.org>.
Github user kdubb commented on the pull request:
https://github.com/apache/thrift/pull/790#issuecomment-172646482
I am fairly sure that #797 fixes the line breaks and semi-colons in the proper way. So, just check it with your code to make sure.
As far as performance of ARC I will just say that you attempt to be micro optimizing code that is about to, or just has, sent data over a network connection. Any thought to the nanosecond of time ARC *may* (and most likely doesn't) add is not something we should be concerned about. Finally, this code initializes static constants so discussing performance at all is unnecessary.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---