Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289173002:
Just skip deployment target checks for OS versions newer than our SDK (Closed)
|
Created:
4 years, 3 months ago by Sidney San Martín Modified:
4 years, 3 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionJust skip deployment target checks for OS versions newer than our SDK
Instead of guessing what *would* have been the OS X version constant for
a version our SDK doesn't even support and then asserting when we're
wrong, just skip the check when it doesn't matter.
BUG=636093
Committed: https://crrev.com/9745ad33c757b587eade6247bff56379576c6fa7
Cr-Commit-Position: refs/heads/master@{#415391}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Drop _ prefix, CHECK -> TEST #
Depends on Patchset: Messages
Total messages: 21 (10 generated)
Description was changed from ========== Just skip deployment target checks for OS versions newer than our SDK BUG=636093 ========== to ========== Just skip deployment target checks for OS versions newer than our SDK Instead of guessing what *would* have been the OS X version constant for a version our SDK doesn't even support and then asserting when we're wrong, just skip the check when it doesn't matter. BUG=636093 ==========
Description was changed from ========== Just skip deployment target checks for OS versions newer than our SDK Instead of guessing what *would* have been the OS X version constant for a version our SDK doesn't even support and then asserting when we're wrong, just skip the check when it doesn't matter. BUG=636093 ========== to ========== Just skip deployment target checks for OS versions newer than our SDK Instead of guessing what *would* have been the OS X version constant for a version our SDK doesn't even support and then asserting when we're wrong, just skip the check when it doesn't matter. BUG=636093 ==========
sdy@chromium.org changed reviewers: + mark@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
I promise, I'll put this to bed soon :).
I’m concerned that the layers of macro indirection are harder to understand now, but it’s nice to be able to do this “cleanly” and just as correctly without having to offer our own 10xx00 constant, so LGTM with these small changes. https://codereview.chromium.org/2289173002/diff/60001/base/mac/mac_util.h File base/mac/mac_util.h (right): https://codereview.chromium.org/2289173002/diff/60001/base/mac/mac_util.h#new... base/mac/mac_util.h:123: #define _DEFINE_IS_OS_FUNCS(V, CHECK_TARGET) \ This is just DEFINE_IS_OS_FUNCS now, there’s no “outer” version that calls it. [CHECK_]DEPLOYMENT_TARGET instead of just TARGET, for clarity, throughout. Don’t use CHECK, though, because then it makes it seem like it’s a base/logging.h CHECK, and it’s not.
Mark Mentovai wrote: > Don’t use CHECK, though, because then it makes it seem like it’s a > base/logging.h CHECK, and it’s not. (TEST instead of CHECK?)
https://codereview.chromium.org/2289173002/diff/60001/base/mac/mac_util.h File base/mac/mac_util.h (right): https://codereview.chromium.org/2289173002/diff/60001/base/mac/mac_util.h#new... base/mac/mac_util.h:123: #define _DEFINE_IS_OS_FUNCS(V, CHECK_TARGET) \ On 2016/08/30 16:08:15, Mark Mentovai wrote: > This is just DEFINE_IS_OS_FUNCS now, there’s no “outer” version that calls it. Since there are a few internal macros now, I was looking for a convention to mark these are as file-private (and avoid stomping a different TEST_DEPLOYMENT_TARGET that's intended to be global, should there ever be one). Let me know if there is one, but for now I'll just drop the _s.
If you #undef them when you're done, as you do, you don't need to do anything like underscores to discourage use outside this file.
On 2016/08/30 17:14:42, Mark Mentovai wrote: > If you #undef them when you're done, as you do, you don't need to do anything > like underscores to discourage use outside this file. I more meant the case where another header uses the same name for an exported macro, making the inclusion order matter.
On 2016/08/30 17:19:28, Sidney San Martín wrote: > On 2016/08/30 17:14:42, Mark Mentovai wrote: > > If you #undef them when you're done, as you do, you don't need to do anything > > like underscores to discourage use outside this file. > > I more meant the case where another header uses the same name for an exported > macro, making the inclusion order matter. The preprocessor would warn if that happened, provided that the two definitions were different. Since there isn't really anything standard about these macros, the likelihood of the definitions being the same is exceedingly low. (STR, XSTR, and CONCAT are admittedly kind of a different story, but you aren't using those now.) It's basically impossible to wind up in this situation. If you are still concerned, consider psuedo-i namespacing the macros with a MAC_UTIL or even BASE_MAC_MAC_UTIL prefix.
On 2016/08/30 17:51:55, Mark Mentovai wrote: > The preprocessor would warn if that happened, provided that the two definitions > were different. Since there isn't really anything standard about these macros, > the likelihood of the definitions being the same is exceedingly low. (STR, XSTR, > and CONCAT are admittedly kind of a different story, but you aren't using those > now.) > > It's basically impossible to wind up in this situation. > > If you are still concerned, consider psuedo-i namespacing the macros with a > MAC_UTIL or even BASE_MAC_MAC_UTIL prefix. That's reasonable (to say that these aren't conflict-prone names). I tried that style of prefix earlier, but it made things *extra* hard to read :). I was looking for something short with a similar meaning to static, where it'd be plausible for a linter to complain if I forget an #undef (even if it doesn't right now), and it would be obvious to a reader who hasn't made it to the #undefs that these macros aren't part of mac_util's interface.
The CQ bit was checked by sdy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2289173002/#ps80001 (title: "Drop _ prefix, CHECK -> TEST")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Just skip deployment target checks for OS versions newer than our SDK Instead of guessing what *would* have been the OS X version constant for a version our SDK doesn't even support and then asserting when we're wrong, just skip the check when it doesn't matter. BUG=636093 ========== to ========== Just skip deployment target checks for OS versions newer than our SDK Instead of guessing what *would* have been the OS X version constant for a version our SDK doesn't even support and then asserting when we're wrong, just skip the check when it doesn't matter. BUG=636093 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Just skip deployment target checks for OS versions newer than our SDK Instead of guessing what *would* have been the OS X version constant for a version our SDK doesn't even support and then asserting when we're wrong, just skip the check when it doesn't matter. BUG=636093 ========== to ========== Just skip deployment target checks for OS versions newer than our SDK Instead of guessing what *would* have been the OS X version constant for a version our SDK doesn't even support and then asserting when we're wrong, just skip the check when it doesn't matter. BUG=636093 Committed: https://crrev.com/9745ad33c757b587eade6247bff56379576c6fa7 Cr-Commit-Position: refs/heads/master@{#415391} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9745ad33c757b587eade6247bff56379576c6fa7 Cr-Commit-Position: refs/heads/master@{#415391} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
