Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2135453002:
Promote the GCC/libc++/<memory> 'typedef float float32_t' workaround to SkTypes.h (Closed)
DescriptionPromote the GCC/libc++/<memory> 'typedef float float32_t' workaround to SkTypes.h
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2135453002
Committed: https://skia.googlesource.com/skia/+/f48c62fa6ae703f0e4fa7b97a381eb06afaadc4b
Patch Set 1 #Patch Set 2 : update #Patch Set 3 : update comment #Patch Set 4 : add tab #
Total comments: 11
Patch Set 5 : Address comments #
Messages
Total messages: 29 (13 generated)
Description was changed from ========== Promote the GCC/libc++/<memory> 'typedef float float32_t' workaround to SkTypes.h BUG=skia: ========== to ========== Promote the GCC/libc++/<memory> 'typedef float float32_t' workaround to SkTypes.h BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2135453002 ==========
bsalomon@google.com changed reviewers: + reed@google.com
Description was changed from ========== Promote the GCC/libc++/<memory> 'typedef float float32_t' workaround to SkTypes.h BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2135453002 ========== to ========== Promote the GCC/libc++/<memory> 'typedef float float32_t' workaround to SkTypes.h BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2135453002 ==========
bsalomon@google.com changed reviewers: + bungeman@google.com, mtklein@google.com
The CQ bit was checked by bsalomon@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-07-08 05:20 UTC
The CQ bit was unchecked by bsalomon@google.com
The CQ bit was checked by reed@google.com to run a CQ dry run
lgtm, but would like a review from either/both bunge | klein
The CQ bit was checked by bsalomon@google.com 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...
The CQ bit was checked by reed@google.com 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...
The CQ bit was unchecked by reed@google.com
https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:23: && defined(_LIBCPP_VERSION) I changed this logic slightly. It now checks for ARM32+NEON or ARM64 and gcc minor revision <= 9 since this reproduced on 64 bit ARM builds using gcc 4.9.?. Locally I could only reproduce it in a debug build, but one of the bots reproduced it in a release build.
https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:25: #include <memory> If I didn't #include <memory> here then this didn't work for me in clank build. This is despite the fact that the error was occurring due to SkTemplates.h including <memory> and SkTemplates.h includes this file first.
everything but the presumed typo lgtm https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:21: #if defined(__GNUC__) && __GNUC__ == 4 && __GNUC_MINOR__ <= 9 \ Probably would also be okay to just drop the GNUC_MINOR check. 4.9 is the last 4-series GCC, so <= 9 should always be true. This reads just fine as is too. https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:33: // IWYU pragma: end_exportsZ typo?
https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:25: #include <memory> On 2016/07/07 23:29:03, bsalomon wrote: > If I didn't #include <memory> here then this didn't work for me in clank build. > This is despite the fact that the error was occurring due to SkTemplates.h > including <memory> and SkTemplates.h includes this file first. Yeah, you've got to get that typedef float float32_t in before the first transitive <memory> include. <memory> is getting pretty pervasive in our own codebase, and there's no guarantees about what standard library headers include each other, so any #include <foo> is a potential #include <memory>.
https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:19: // /should just let die. stray slash here too
https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:19: // /should just let die. On 2016/07/07 23:32:41, mtklein wrote: > stray slash here too Done. https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:21: #if defined(__GNUC__) && __GNUC__ == 4 && __GNUC_MINOR__ <= 9 \ On 2016/07/07 23:30:12, mtklein wrote: > Probably would also be okay to just drop the GNUC_MINOR check. 4.9 is the last > 4-series GCC, so <= 9 should always be true. This reads just fine as is too. Done. https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:25: #include <memory> On 2016/07/07 23:32:03, mtklein wrote: > On 2016/07/07 23:29:03, bsalomon wrote: > > If I didn't #include <memory> here then this didn't work for me in clank > build. > > This is despite the fact that the error was occurring due to SkTemplates.h > > including <memory> and SkTemplates.h includes this file first. > > Yeah, you've got to get that typedef float float32_t in before the first > transitive <memory> include. <memory> is getting pretty pervasive in our own > codebase, and there's no guarantees about what standard library headers include > each other, so any #include <foo> is a potential #include <memory>. Yep, but the compiler error gave the #include stack and pointed at the one in SkTemplates.h. I spotted no instances in that chain of including std headers before SkTypes.h. *shrug* https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:33: // IWYU pragma: end_exportsZ On 2016/07/07 23:30:12, mtklein wrote: > typo? Done.
The CQ bit was checked by bsalomon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2135453002/#ps80001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/2135453002/diff/60001/include/core/SkTypes.h#... include/core/SkTypes.h:25: #include <memory> On 2016/07/07 23:47:08, bsalomon wrote: > *shrug* Indeed.
Message was sent while issue was closed.
Description was changed from ========== Promote the GCC/libc++/<memory> 'typedef float float32_t' workaround to SkTypes.h BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2135453002 ========== to ========== Promote the GCC/libc++/<memory> 'typedef float float32_t' workaround to SkTypes.h BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2135453002 Committed: https://skia.googlesource.com/skia/+/f48c62fa6ae703f0e4fa7b97a381eb06afaadc4b ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/f48c62fa6ae703f0e4fa7b97a381eb06afaadc4b
Message was sent while issue was closed.
CQ bit was unchecked. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
