Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8528013:
Convert plain C-style casts to use CFCastStrict and GetValueFromDictionary template (Closed)
|
Created:
9 years, 1 month ago by KushalP Modified:
9 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Visibility:
Public. |
DescriptionConvert plain C-style casts to use CFCastStrict and GetValueFromDictionary template.
These updates keep CF casts between CFTypeRef and specific CoreFoundation types consistent across the project.
BUG=104200
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110173
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed alignment. Removed DCHECKs when CFCastStrict is used. #Patch Set 3 : Line alignment + Switching strict cases to normal. #
Total comments: 7
Patch Set 4 : Add updated GetValueFromDictionary, and update CFCasts #
Total comments: 8
Patch Set 5 : Strict casts, fix DCHECKs, and alignment #
Total comments: 2
Patch Set 6 : Fix DCHECKs #
Total comments: 5
Patch Set 7 : alignment #
Messages
Total messages: 26 (0 generated)
Currently running through the trybots. The win failure is not related to my change and it might just need a clobber. Asked in the IRC chan and this was confirmed.
I don't understand these casts. Mark, can you look? If it's OK with you, then I'll rubberstamp this.
drive-by comments http://codereview.chromium.org/8528013/diff/1/net/proxy/proxy_config_service_... File net/proxy/proxy_config_service_mac.cc (right): http://codereview.chromium.org/8528013/diff/1/net/proxy/proxy_config_service_... net/proxy/proxy_config_service_mac.cc:33: base::mac::CFCastStrict<CFNumberRef>( nit: You can align this to the previous line, and then you only have to indent line 34 by four spaces, rather than 8. This also applies to the following lines: 65-70, 137-140 However, I don't believe any of these casts are needed to be strict (except for 144/145), as GetValueFromDictionary already asserts that the returned type is the same as the expected type. If they aren't, it will DLOG(ERROR) and return NULL, which accomplishes the same as the Strict check. So perhaps just CFCast these? http://codereview.chromium.org/8528013/diff/1/net/proxy/proxy_resolver_mac.cc File net/proxy/proxy_resolver_mac.cc (right): http://codereview.chromium.org/8528013/diff/1/net/proxy/proxy_resolver_mac.cc... net/proxy/proxy_resolver_mac.cc:146: base::mac::CFCastStrict<CFDictionaryRef>( nit: You can also re-align here and 165-168
http://codereview.chromium.org/8528013/diff/1/net/proxy/proxy_config_service_... File net/proxy/proxy_config_service_mac.cc (right): http://codereview.chromium.org/8528013/diff/1/net/proxy/proxy_config_service_... net/proxy/proxy_config_service_mac.cc:33: base::mac::CFCastStrict<CFNumberRef>( In the case of GetValueFromDictionary() it should definitely use the Strict case as we've already partially told it what to expect in the bucket. I'll update the CFArrayGetValueAtIndex cases to use the non-strict variant. On 2011/11/11 00:26:42, Ryan Sleevi wrote: > nit: You can align this to the previous line, and then you only have to indent > line 34 by four spaces, rather than 8. > > This also applies to the following lines: 65-70, 137-140 > > However, I don't believe any of these casts are needed to be strict (except for > 144/145), as GetValueFromDictionary already asserts that the returned type is > the same as the expected type. > > If they aren't, it will DLOG(ERROR) and return NULL, which accomplishes the same > as the Strict check. > > So perhaps just CFCast these?
http://codereview.chromium.org/8528013/diff/1/net/proxy/proxy_config_service_... File net/proxy/proxy_config_service_mac.cc (right): http://codereview.chromium.org/8528013/diff/1/net/proxy/proxy_config_service_... net/proxy/proxy_config_service_mac.cc:33: base::mac::CFCastStrict<CFNumberRef>( On 2011/11/11 11:03:19, KushalP wrote: > In the case of GetValueFromDictionary() it should definitely use the Strict case > as we've already partially told it what to expect in the bucket. > > I'll update the CFArrayGetValueAtIndex cases to use the non-strict variant. > I think you misunderstood. I believe CFArrayGetValueAtIndex *should* be using the strict. However, please examine the implementation of GetValueFromDictionary. You'll see that it's already doing strict checks - if the value it extracts from the dictionary is not of the same CFType requested, it will DLOG(WARNING) and return NULL.
http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_config_servi... File net/proxy/proxy_config_service_mac.cc (right): http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_config_servi... net/proxy/proxy_config_service_mac.cc:33: base::mac::GetValueFromDictionary(dict, key, CFNumberGetTypeID())); I would only consider this a temporary measure. GetValueFromDictionary now requires you to specify the type in two different ways: once as the third argument and once in the cast as you store the value in an appropriately-typed variable. That’s not cool. I think that GetValueFromDictionary should be templatized to return the proper type. http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_config_servi... net/proxy/proxy_config_service_mac.cc:144: if (CFGetTypeID(bypass_item_ref) != CFStringGetTypeID()) { This is wrong now. bypass_item_ref will either be a pointer to a CFString, or it will be NULL if the value in the array was not a CFString. This check can become if (!bypass_item_ref) { http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_resolver_mac.cc File net/proxy/proxy_resolver_mac.cc (right): http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_resolver_mac... net/proxy/proxy_resolver_mac.cc:134: base::mac::CFCastStrict<CFArrayRef>(result)); The old code has this as a DCHECK, but you’ve used a strict cast which is equivalent to CHECK, with the difference that it will permit NULL. You need to carefully consider how to transform the old code, and recognize that if you’re changing what’s CHECKed or DCHECKed, or changing between D and non-D, you may need to revise some of the surrounding code. http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_resolver_mac... net/proxy/proxy_resolver_mac.cc:145: CFArrayGetValueAtIndex(proxy_array_ref.get(), i)); Same. http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_resolver_mac... net/proxy/proxy_resolver_mac.cc:161: CFStringRef proxy_type = base::mac::CFCastStrict<CFStringRef>( And in this case, the old code didn’t even have a DCHECK, but you’re moving to a strict cast. For all of these, what you’ve written as-is may be OK, but you should explain (in the review, not a comment) why you’ve made a behavior change. “The old code would have crashed anyway in all of the cases where the strict cast now CHECKs” would be a perfectly fine answer, if it’s true. http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_server_mac.cc File net/proxy/proxy_server_mac.cc (right): http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_server_mac.c... net/proxy/proxy_server_mac.cc:28: base::mac::GetValueFromDictionary(dict, host_key, CFStringGetTypeID())); Yeah, I really think we need a better API for GetValueFromDictionary. You might want to do that before changing these.
Updated with new GetValueFromDictionary and added CHECKs for NULL values.
http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_config_serv... File net/proxy/proxy_config_service_mac.cc (right): http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_mac.cc:32: CFNumberRef number = base::mac::GetValueFromDictionary<CFNumberRef>( dict will fit on this line now, and key can go right beneath it. http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_mac.cc:137: CFArrayGetValueAtIndex(bypass_array_ref, i)); Why are you reindenting this line? It was better where it was before. http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_resolver_ma... File net/proxy/proxy_resolver_mac.cc (right): http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:126: DCHECK(!result); This used to be DCHECK(result != NULL) and you’ve changed it to DCHECK(result), which has the exact opposite meaning. http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:135: DCHECK(!result); This, too, appears to be the wrong condition to DCHECK. Have you tested this? http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:147: DCHECK(!proxy_dictionary); And once more.
http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_resolver_ma... File net/proxy/proxy_resolver_mac.cc (right): http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:126: DCHECK(!result); On 2011/11/15 14:23:20, Mark Mentovai wrote: > This used to be DCHECK(result != NULL) and you’ve changed it to DCHECK(result), > which has the exact opposite meaning. Yup. What I did is the same as DCHECK(result == NULL). Fixing. http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:135: DCHECK(!result); On 2011/11/15 14:23:20, Mark Mentovai wrote: > This, too, appears to be the wrong condition to DCHECK. Have you tested this? Will change this to CFCastStrict and add a check for NULL. I tested locally and ran through the trybots which appears green. Should I also add tests for this class?
Updated with Patch Set 5.
http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_resolver_ma... File net/proxy/proxy_resolver_mac.cc (right): http://codereview.chromium.org/8528013/diff/11001/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:135: DCHECK(!result); KushalP wrote: > Should I also add tests for this class? Probably not necessary. http://codereview.chromium.org/8528013/diff/16001/net/proxy/proxy_resolver_ma... File net/proxy/proxy_resolver_mac.cc (right): http://codereview.chromium.org/8528013/diff/16001/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:126: DCHECK(!result); I thought you said you fixed this. This is a simple but critical error that I already pointed out and that a quick inspection would have revealed. If you’re not going to take the time to pre-review your changes before sending them out for review, why should I take the time to review them?
http://codereview.chromium.org/8528013/diff/16001/net/proxy/proxy_resolver_ma... File net/proxy/proxy_resolver_mac.cc (right): http://codereview.chromium.org/8528013/diff/16001/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:126: DCHECK(!result); On 2011/11/15 17:35:28, Mark Mentovai wrote: > I thought you said you fixed this. This is a simple but critical error that I > already pointed out and that a quick inspection would have revealed. > > If you’re not going to take the time to pre-review your changes before sending > them out for review, why should I take the time to review them? My mistake. I'm sorry.
http://codereview.chromium.org/8528013/diff/15003/net/proxy/proxy_server_mac.cc File net/proxy/proxy_server_mac.cc (right): http://codereview.chromium.org/8528013/diff/15003/net/proxy/proxy_server_mac.... net/proxy/proxy_server_mac.cc:27: CFStringRef host_ref = base::mac::GetValueFromDictionary<CFStringRef>( Don’t you think that this would be more readable in the same style that the existing code used? CFStringRef host_ref = base::mac::GetValueFromDictionary<CFStringRef>(dict, host_key); Same on lines 37-38.
http://codereview.chromium.org/8528013/diff/15003/net/proxy/proxy_server_mac.cc File net/proxy/proxy_server_mac.cc (right): http://codereview.chromium.org/8528013/diff/15003/net/proxy/proxy_server_mac.... net/proxy/proxy_server_mac.cc:27: CFStringRef host_ref = base::mac::GetValueFromDictionary<CFStringRef>( On 2011/11/15 19:18:21, Mark Mentovai wrote: > Don’t you think that this would be more readable in the same style that the > existing code used? > > CFStringRef host_ref = > base::mac::GetValueFromDictionary<CFStringRef>(dict, host_key); > > Same on lines 37-38. I do, but doing so would mean both lines end up being 83 characters long and fail the presubmit checks. So I went for what seemed natural off the style guide: http://www.chromium.org/developers/coding-style#TOC-Code-formatting (the line in green starting with "bool result =")
On 2011/11/15 19:25:59, KushalP wrote: > > I do, but doing so would mean both lines end up being 83 characters long and > fail the presubmit checks. > > So I went for what seemed natural off the style guide: > http://www.chromium.org/developers/coding-style#TOC-Code-formatting (the line in > green starting with "bool result =") Should I instead push the function on the same line as the variable and move the parameters down to the next line?
http://codereview.chromium.org/8528013/diff/15003/net/proxy/proxy_server_mac.cc File net/proxy/proxy_server_mac.cc (right): http://codereview.chromium.org/8528013/diff/15003/net/proxy/proxy_server_mac.... net/proxy/proxy_server_mac.cc:27: CFStringRef host_ref = base::mac::GetValueFromDictionary<CFStringRef>( KushalP wrote: > On 2011/11/15 19:18:21, Mark Mentovai wrote: > > Don’t you think that this would be more readable in the same style that the > > existing code used? > > > > CFStringRef host_ref = > > base::mac::GetValueFromDictionary<CFStringRef>(dict, host_key); > > > > Same on lines 37-38. > > I do, but doing so would mean both lines end up being 83 characters long and > fail the presubmit checks. > > So I went for what seemed natural off the style guide: > http://www.chromium.org/developers/coding-style#TOC-Code-formatting (the line in > green starting with "bool result =") The example I gave you, quoted above and reproduced here: CFStringRef host_ref = base::mac::GetValueFromDictionary<CFStringRef>(dict, host_key); is only 69 columns wide. You’re still using two lines, you’re just putting the break in a place that makes the code read more naturally. In fact, what I’m suggesting is exactly the same as the advice given by the Chromium style guide in the section you referenced. http://codereview.chromium.org/8528013/diff/15003/net/proxy/proxy_server_mac.... net/proxy/proxy_server_mac.cc:37: CFNumberRef port_ref = base::mac::GetValueFromDictionary<CFNumberRef>( CFNumberRef port_ref = base::mac::GetValueFromDictionary<CFNumberRef>(dict, port_key); would also be 69 columns on two lines.
http://codereview.chromium.org/8528013/diff/15003/net/proxy/proxy_server_mac.cc File net/proxy/proxy_server_mac.cc (right): http://codereview.chromium.org/8528013/diff/15003/net/proxy/proxy_server_mac.... net/proxy/proxy_server_mac.cc:27: CFStringRef host_ref = base::mac::GetValueFromDictionary<CFStringRef>( On 2011/11/15 19:40:56, Mark Mentovai wrote: > KushalP wrote: > > On 2011/11/15 19:18:21, Mark Mentovai wrote: > > > Don’t you think that this would be more readable in the same style that the > > > existing code used? > > > > > > CFStringRef host_ref = > > > base::mac::GetValueFromDictionary<CFStringRef>(dict, host_key); > > > > > > Same on lines 37-38. > > > > I do, but doing so would mean both lines end up being 83 characters long and > > fail the presubmit checks. > > > > So I went for what seemed natural off the style guide: > > http://www.chromium.org/developers/coding-style#TOC-Code-formatting (the line > in > > green starting with "bool result =") > > The example I gave you, quoted above and reproduced here: > > CFStringRef host_ref = > base::mac::GetValueFromDictionary<CFStringRef>(dict, host_key); > > is only 69 columns wide. > > You’re still using two lines, you’re just putting the break in a place that > makes the code read more naturally. > > In fact, what I’m suggesting is exactly the same as the advice given by the > Chromium style guide in the section you referenced. Fixed now. Should I also add an extra newline at lines 29 and 38 now? It doesn't look like a new 'paragraph' but it does read cleaner around the if statement. It worked previously as the final param was pushed to the far-right making it look like a new line.
This one’s good. No extra newlines needed. LGTM and CQ.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kushi.p@gmail.com/8528013/12007
Presubmit check for 8528013-12007 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: net/proxy/proxy_resolver_mac.cc,net/proxy/proxy_server_mac.cc,net/proxy/proxy_config_service_mac.cc Presubmit checks took 1.6s to calculate.
LGTM rubberstamp On Tue, Nov 15, 2011 at 3:08 PM, <commit-bot@chromium.org> wrote: > Presubmit check for 8528013-12007 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit Messages ** > If this change requires manual test instructions to QA team, add > TEST=[instructions]. > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for: > net/proxy/proxy_resolver_mac.**cc,net/proxy/proxy_server_mac.** > cc,net/proxy/proxy_config_**service_mac.cc > > Presubmit checks took 1.6s to calculate. > > > > http://codereview.chromium.**org/8528013/<http://codereview.chromium.org/8528... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kushi.p@gmail.com/8528013/12007
Change committed as 110173
Patch Set 7 LGTM. Thanks. I just have a suggestion below. http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_config_servi... File net/proxy/proxy_config_service_mac.cc (right): http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_config_servi... net/proxy/proxy_config_service_mac.cc:12: #include "base/mac/mac_util.h" Can you see if we can remove this #include line now?
On 2011/11/16 06:23:32, wtc wrote: > Patch Set 7 LGTM. Thanks. I just have a suggestion below. > > http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_config_servi... > File net/proxy/proxy_config_service_mac.cc (right): > > http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_config_servi... > net/proxy/proxy_config_service_mac.cc:12: #include "base/mac/mac_util.h" > > Can you see if we can remove this #include line now? Sure. Can that be done as part of this CL or do I need to make a new issue?
On 2011/11/16 09:24:46, KushalP wrote: > On 2011/11/16 06:23:32, wtc wrote: > > Patch Set 7 LGTM. Thanks. I just have a suggestion below. > > > > > http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_config_servi... > > File net/proxy/proxy_config_service_mac.cc (right): > > > > > http://codereview.chromium.org/8528013/diff/8001/net/proxy/proxy_config_servi... > > net/proxy/proxy_config_service_mac.cc:12: #include "base/mac/mac_util.h" > > > > Can you see if we can remove this #include line now? > > Sure. Can that be done as part of this CL or do I need to make a new issue? Do it as a new CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
