Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2473153003:
Move some compiler intrinsic #defines to base/. (Closed)
DescriptionMove some compiler intrinsic #defines to base/.
PartitionAlloc uses these, and we are moving PA to base/.
BUG=632441
Committed: https://crrev.com/58184a8289cf6ae9cbe623b0c50f7e226e8bb35b
Cr-Commit-Position: refs/heads/master@{#430681}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move NEVER_INLINE to Blink, add a TODO. #Patch Set 3 : Don't redefine ALWAYS_INLINE in ios/... . #
Total comments: 5
Dependent Patchsets: Messages
Total messages: 30 (13 generated)
palmer@chromium.org changed reviewers: + thakis@chromium.org
thakis: You are the OWNER of both areas, so I picked you. It should be a trivial review. This is the minimal change; I could move more or all of it you think that's better. For context, see https://docs.google.com/document/d/16FROhiOc0eAg58Kn30mERT96I2eYOvTF8VDi6cwkv....
thakis: You are the OWNER of both areas, so I picked you. It should be a trivial review. This is the minimal change; I could move more or all of it you think that's better. For context, see https://docs.google.com/document/d/16FROhiOc0eAg58Kn30mERT96I2eYOvTF8VDi6cwkv....
palmer@chromium.org changed reviewers: + dcheng@chromium.org, esprehn@chromium.org
Adding dcheng and esprehn for Pacific-time felicity.
lgtm
LGTM
https://codereview.chromium.org/2473153003/diff/1/base/compiler_specific.h File base/compiler_specific.h (right): https://codereview.chromium.org/2473153003/diff/1/base/compiler_specific.h#ne... base/compiler_specific.h:104: #define NEVER_INLINE NOINLINE Btw, maybe a TODO here for eventual convergence. I would probably move this #define into Blink actually (so that stuff outside Blink doesn't start depending on the alias) I guess we'll need to change PA when we move it out, but that doesn't seem too bad.
https://codereview.chromium.org/2473153003/diff/1/base/compiler_specific.h File base/compiler_specific.h (right): https://codereview.chromium.org/2473153003/diff/1/base/compiler_specific.h#ne... base/compiler_specific.h:104: #define NEVER_INLINE NOINLINE > Btw, maybe a TODO here for eventual convergence. I would probably move this #define into Blink actually (so that stuff outside Blink doesn't start depending on the alias) > > I guess we'll need to change PA when we move it out, but that doesn't seem too bad. Done.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2473153003/#ps20001 (title: "Move NEVER_INLINE to Blink, add a TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
palmer@chromium.org changed reviewers: + rohitrao@chromium.org
+rohitrao for ios/.
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rohitrao@chromium.org changed reviewers: + sdefresne@chromium.org
+sdefresne to confirm that the switch to just "inline" will be ok for iOS. I'd also be happy to rubberstamp if thakis@ confirms that the change will be ok for iOS. https://codereview.chromium.org/2473153003/diff/40001/ios/third_party/blink/s... File ios/third_party/blink/src/html_tokenizer_adapter.h (left): https://codereview.chromium.org/2473153003/diff/40001/ios/third_party/blink/s... ios/third_party/blink/src/html_tokenizer_adapter.h:14: #define ALWAYS_INLINE inline __attribute__((always_inline)) Is this being changed from "inline __attribute__((always_inline))" to just "inline"?
https://codereview.chromium.org/2473153003/diff/40001/ios/third_party/blink/s... File ios/third_party/blink/src/html_tokenizer_adapter.h (left): https://codereview.chromium.org/2473153003/diff/40001/ios/third_party/blink/s... ios/third_party/blink/src/html_tokenizer_adapter.h:14: #define ALWAYS_INLINE inline __attribute__((always_inline)) > Is this being changed from "inline __attribute__((always_inline))" to just "inline"? No, no change. compiler_specific.h says: #if COMPILER_GCC && defined(NDEBUG) #define ALWAYS_INLINE inline __attribute__((__always_inline__)) #elif COMPILER_MSVC && defined(NDEBUG) #define ALWAYS_INLINE __forceinline #else #define ALWAYS_INLINE inline #endif build/build_config.h shows only those 2 compilers as normal, and COMPILER_GCC defined if __GNUC__ is defined, so I am assuming that this covers Clang too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks fine for ios/. lgtm https://codereview.chromium.org/2473153003/diff/40001/base/compiler_specific.h File base/compiler_specific.h (right): https://codereview.chromium.org/2473153003/diff/40001/base/compiler_specific.... base/compiler_specific.h:209: #define LIKELY(x) __builtin_expect((x), 1) Shouldn't this be __builtin_expect(!!(x), 1)? Otherwise, the following will be fails the LIKELY: Foo* ptr = new Foo; if (LIKELY(ptr)) { ... } https://codereview.chromium.org/2473153003/diff/40001/ios/third_party/blink/s... File ios/third_party/blink/src/html_tokenizer_adapter.h (left): https://codereview.chromium.org/2473153003/diff/40001/ios/third_party/blink/s... ios/third_party/blink/src/html_tokenizer_adapter.h:14: #define ALWAYS_INLINE inline __attribute__((always_inline)) On 2016/11/07 23:29:51, palmer wrote: > > Is this being changed from "inline __attribute__((always_inline))" to just > "inline"? > > No, no change. compiler_specific.h says: > > #if COMPILER_GCC && defined(NDEBUG) > #define ALWAYS_INLINE inline __attribute__((__always_inline__)) > #elif COMPILER_MSVC && defined(NDEBUG) > #define ALWAYS_INLINE __forceinline > #else > #define ALWAYS_INLINE inline > #endif > > build/build_config.h shows only those 2 compilers as normal, and COMPILER_GCC > defined if __GNUC__ is defined, so I am assuming that this covers Clang too. Yeah, clang defines __GNUC__: $ clang -dM -E - < /dev/null|grep GNUC #define __GNUC_MINOR__ 2 #define __GNUC_PATCHLEVEL__ 1 #define __GNUC_STDC_INLINE__ 1 #define __GNUC__ 4
lgtm
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2473153003/#ps40001 (title: "Don't redefine ALWAYS_INLINE in ios/... .")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Move some compiler intrinsic #defines to base/. PartitionAlloc uses these, and we are moving PA to base/. BUG=632441 ========== to ========== Move some compiler intrinsic #defines to base/. PartitionAlloc uses these, and we are moving PA to base/. BUG=632441 Committed: https://crrev.com/58184a8289cf6ae9cbe623b0c50f7e226e8bb35b Cr-Commit-Position: refs/heads/master@{#430681} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/58184a8289cf6ae9cbe623b0c50f7e226e8bb35b Cr-Commit-Position: refs/heads/master@{#430681}
Message was sent while issue was closed.
https://codereview.chromium.org/2473153003/diff/40001/base/compiler_specific.h File base/compiler_specific.h (right): https://codereview.chromium.org/2473153003/diff/40001/base/compiler_specific.... base/compiler_specific.h:209: #define LIKELY(x) __builtin_expect((x), 1) > Shouldn't this be __builtin_expect(!!(x), 1)? Otherwise, the following will be fails the LIKELY: > > Foo* ptr = new Foo; > if (LIKELY(ptr)) { > ... > } Somehow, I missed this comment. Fix coming right up. Thanks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
