Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
255393002:
Hide NSAutoreleasePool from ARC translation units. (Closed)
|
Created:
6 years, 8 months ago by jfroy Modified:
6 years, 7 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionHide NSAutoreleasePool from ARC translation units.
BUG=366312
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267067
Patch Set 1 #Patch Set 2 : Hide the autorelease pool type instead of methods in ARC TUs to keep the vtable compatible. #Patch Set 3 : Update the preprocessor conditional comment with the right expression. #Patch Set 4 : Change the return type in the method definitions to match the declarations. #
Total comments: 1
Patch Set 5 : Rename type alias to AutoreleasePoolType and add comment as to its purpose. #
Total comments: 1
Patch Set 6 : Updated comment about AutoreleasePoolType to address feedback. #Patch Set 7 : Rebase #Patch Set 8 : Rebase #Patch Set 9 : Rebase #Messages
Total messages: 31 (0 generated)
Does this actually work? It seems weird to conditionally add/remove methods in a TU depending on a TU-specific feature.
I haven't ran into any issues with this, but come to think of it removing a virtual method in some TUs is not a good idea. Instead, the type should be obfuscated in a manner similar to what's done for pure C++ TUs (see the top of the file). I'll send another patch doing that. On Fri, Apr 25, 2014 at 8:50, <rsesek@chromium.org> wrote: > Does this actually work? It seems weird to conditionally add/remove methods > in a > TU depending on a TU-specific feature. > > https://codereview.chromium.org/255393002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/25 16:36:13, jfroy_google.com wrote: > I haven't ran into any issues with this, but come to think of it removing a > virtual method in some TUs is not a good idea. Instead, the type should be > obfuscated in a manner similar to what's done for pure C++ TUs (see the top > of the file). I'll send another patch doing that. > On Fri, Apr 25, 2014 at 8:50, <mailto:rsesek@chromium.org> wrote: Right, I was concerned about potentially doing strange things to the vtable if methods exist only exist in some TUs and not others. Making the type opaque seems like the right thing to do.
You should also change the type in the .mm file to use the typedef.
On 2014/04/28 19:05:12, rsesek wrote: > You should also change the type in the .mm file to use the typedef. I can do that. AFAIK the return type does not participate in symbol mangling and chromium is unlikely to convert to ARC ever. but it's good for consistency.
LGTM. mark@ is the right OWNER for this file, though. You should also let reviewers know with an explicit message when to take a look.
https://codereview.chromium.org/255393002/diff/50001/base/message_loop/messag... File base/message_loop/message_pump_mac.h (right): https://codereview.chromium.org/255393002/diff/50001/base/message_loop/messag... base/message_loop/message_pump_mac.h:65: class AutoreleasePool; Call this AutoreleasePoolType so that it doesn’t look like it’s a real class when you read it inline below. You should also provide a comment here explaining what the problem is, because as far as I can see, you’re just renaming a type. Does mixing ARC-using code with a type named NSAutoreleasePool cause a problem?
Thanks Mark, check the latest PS.
LGTM https://codereview.chromium.org/255393002/diff/70001/base/message_loop/messag... File base/message_loop/message_pump_mac.h (right): https://codereview.chromium.org/255393002/diff/70001/base/message_loop/messag... base/message_loop/message_pump_mac.h:65: // depends on the TU in which this header appears. In pure C++ TUs, it is TU is not a commonly-understood abbreviation. Say “translation unit” the first time, and you can leave the rest as TU.
The CQ bit was checked by jfroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/255393002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by jfroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/255393002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by jfroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/255393002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium
On 2014/04/29 22:33:08, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_chromium_rel on tryserver.chromium I am having no luck at all with the bot.
The CQ bit was checked by jfroy@chromium.org
The CQ bit was checked by jfroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/255393002/150001
Message was sent while issue was closed.
Change committed as 267067
Message was sent while issue was closed.
|
