Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1526703003:
Porting Skia for newlib compatibility. (Closed)
|
Created:
5 years ago by Sean Klein Modified:
5 years ago CC:
Mark Seaborn, Petr Hosek, reviews_skia.org Base URL:
https://skia.googlesource.com/skia@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionPorting Skia for newlib compatibility.
"locale_t" is not defined for Newlib.
BUG=https://github.com/domokit/mojo/issues/431
Committed: https://skia.googlesource.com/skia/+/db284c52e62e8d16708e2065495a3b693b238771
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Messages
Total messages: 23 (13 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Porting Skia for newlib compatibility. BUG=skia: ========== to ========== Porting Skia for newlib compatibility. "locale_t" is not defined for Newlib. "timezone" does not exist in Newlib -- but "_timezone" DOES exist. BUG=skia: ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
smklein@chromium.org changed reviewers: + mtklein@chromium.org, mtklein@skia.org
mseaborn@chromium.org changed reviewers: + mseaborn@chromium.org
https://codereview.chromium.org/1526703003/diff/60001/src/gpu/GrAutoLocaleSet... File src/gpu/GrAutoLocaleSetter.h (right): https://codereview.chromium.org/1526703003/diff/60001/src/gpu/GrAutoLocaleSet... src/gpu/GrAutoLocaleSetter.h:41: #elif !defined(SK_BUILD_FOR_ANDROID) && !defined(__UCLIBC__) && !defined(_NEWLIB_VERSION) This line is >80 chars. Can you wrap it, please? It might be better to do something like this: #if defined(SK_BUILD_FOR_ANDROID) || defined(__UCLIBC__) || defined(_NEWLIB_VERSION) # define HAVE_LOCALE_T 0 #else # define HAVE_LOCALE_T 1 #endif and then check HAVE_LOCALE_T later to avoid the repetition. https://codereview.chromium.org/1526703003/diff/60001/src/ports/SkTime_Unix.cpp File src/ports/SkTime_Unix.cpp (right): https://codereview.chromium.org/1526703003/diff/60001/src/ports/SkTime_Unix.c... src/ports/SkTime_Unix.cpp:28: dt->fTimeZoneMinutes = SkToS16(offset - _timezone / 60); You don't need this change. I fixed nacl-newlib to define "timezone". See https://bugs.chromium.org/p/nativeclient/issues/detail?id=3737.
https://codereview.chromium.org/1526703003/diff/60001/src/gpu/GrAutoLocaleSet... File src/gpu/GrAutoLocaleSetter.h (right): https://codereview.chromium.org/1526703003/diff/60001/src/gpu/GrAutoLocaleSet... src/gpu/GrAutoLocaleSetter.h:41: #elif !defined(SK_BUILD_FOR_ANDROID) && !defined(__UCLIBC__) && !defined(_NEWLIB_VERSION) On 2015/12/15 00:27:03, Mark Seaborn wrote: > This line is >80 chars. Can you wrap it, please? > > It might be better to do something like this: > > #if defined(SK_BUILD_FOR_ANDROID) || defined(__UCLIBC__) || > defined(_NEWLIB_VERSION) > # define HAVE_LOCALE_T 0 > #else > # define HAVE_LOCALE_T 1 > #endif > > and then check HAVE_LOCALE_T later to avoid the repetition. Done. https://codereview.chromium.org/1526703003/diff/60001/src/ports/SkTime_Unix.cpp File src/ports/SkTime_Unix.cpp (right): https://codereview.chromium.org/1526703003/diff/60001/src/ports/SkTime_Unix.c... src/ports/SkTime_Unix.cpp:28: dt->fTimeZoneMinutes = SkToS16(offset - _timezone / 60); On 2015/12/15 00:27:03, Mark Seaborn wrote: > You don't need this change. I fixed nacl-newlib to define "timezone". See > https://bugs.chromium.org/p/nativeclient/issues/detail?id=3737. Done.
Description was changed from ========== Porting Skia for newlib compatibility. "locale_t" is not defined for Newlib. "timezone" does not exist in Newlib -- but "_timezone" DOES exist. BUG=skia: ========== to ========== Porting Skia for newlib compatibility. "locale_t" is not defined for Newlib. BUG=skia: ==========
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1526703003/diff/80001/src/gpu/GrAutoLocaleSet... File src/gpu/GrAutoLocaleSetter.h (right): https://codereview.chromium.org/1526703003/diff/80001/src/gpu/GrAutoLocaleSet... src/gpu/GrAutoLocaleSetter.h:25: #if defined(SK_BUILD_FOR_ANDROID) || defined(__UCLIBC__) || \ Skia typically wraps at column 100, so I bet this'll all fit on one line? https://codereview.chromium.org/1526703003/diff/80001/src/gpu/GrAutoLocaleSet... src/gpu/GrAutoLocaleSetter.h:27: #define HAVE_LOCALE_T 0 We tend to indent inside #if blocks, but for something this short it doesn't matter much. https://codereview.chromium.org/1526703003/diff/80001/src/gpu/GrAutoLocaleSet... src/gpu/GrAutoLocaleSetter.h:82: #endif Given that this is a header, let's #undef HAVE_LOCALE_T down here for hygiene?
https://codereview.chromium.org/1526703003/diff/80001/src/gpu/GrAutoLocaleSet... File src/gpu/GrAutoLocaleSetter.h (right): https://codereview.chromium.org/1526703003/diff/80001/src/gpu/GrAutoLocaleSet... src/gpu/GrAutoLocaleSetter.h:25: #if defined(SK_BUILD_FOR_ANDROID) || defined(__UCLIBC__) || \ On 2015/12/15 13:42:58, mtklein wrote: > Skia typically wraps at column 100, so I bet this'll all fit on one line? Done. https://codereview.chromium.org/1526703003/diff/80001/src/gpu/GrAutoLocaleSet... src/gpu/GrAutoLocaleSetter.h:27: #define HAVE_LOCALE_T 0 On 2015/12/15 13:42:58, mtklein wrote: > We tend to indent inside #if blocks, but for something this short it doesn't > matter much. Acknowledged. https://codereview.chromium.org/1526703003/diff/80001/src/gpu/GrAutoLocaleSet... src/gpu/GrAutoLocaleSetter.h:82: #endif On 2015/12/15 13:42:58, mtklein wrote: > Given that this is a header, let's #undef HAVE_LOCALE_T down here for hygiene? Done.
Can you fill out BUG=, please? Then LGTM. https://codereview.chromium.org/1526703003/diff/100001/src/gpu/GrAutoLocaleSe... File src/gpu/GrAutoLocaleSetter.h (right): https://codereview.chromium.org/1526703003/diff/100001/src/gpu/GrAutoLocaleSe... src/gpu/GrAutoLocaleSetter.h:84: #undef HAVE_LOCALE_T Nit: Can you add an empty line after this, please?
Description was changed from ========== Porting Skia for newlib compatibility. "locale_t" is not defined for Newlib. BUG=skia: ========== to ========== Porting Skia for newlib compatibility. "locale_t" is not defined for Newlib. BUG=https://github.com/domokit/mojo/issues/431 ==========
lgtm
https://codereview.chromium.org/1526703003/diff/100001/src/gpu/GrAutoLocaleSe... File src/gpu/GrAutoLocaleSetter.h (right): https://codereview.chromium.org/1526703003/diff/100001/src/gpu/GrAutoLocaleSe... src/gpu/GrAutoLocaleSetter.h:84: #undef HAVE_LOCALE_T On 2015/12/15 18:31:03, Mark Seaborn wrote: > Nit: Can you add an empty line after this, please? Done.
The CQ bit was checked by mtklein@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mseaborn@chromium.org Link to the patchset: https://codereview.chromium.org/1526703003/#ps120001 (title: " ")
The CQ bit was checked by smklein@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1526703003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1526703003/120001
Message was sent while issue was closed.
Description was changed from ========== Porting Skia for newlib compatibility. "locale_t" is not defined for Newlib. BUG=https://github.com/domokit/mojo/issues/431 ========== to ========== Porting Skia for newlib compatibility. "locale_t" is not defined for Newlib. BUG=https://github.com/domokit/mojo/issues/431 Committed: https://skia.googlesource.com/skia/+/db284c52e62e8d16708e2065495a3b693b238771 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://skia.googlesource.com/skia/+/db284c52e62e8d16708e2065495a3b693b238771 |
