Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
7 years ago by Inactive Modified:
7 years ago Reviewers:
Nico, Markus (顧孟勤) CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org, willchan no longer on Chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDisable kSSizeMaxConst COMPILE_ASSERT() for Android / Mac / IOS
The kSSizeMaxConst COMPILE_ASSERT() was previously disabled for clang only, due
to clang giving a build error on Android / Mac / IOS. gcc is affected by the
same problem so this CL disables the COMPILE_ASSERT() for Android / Mac / IOS
platforms, instead of doing it for a specific compiler.
The reason why this COMPILE_ASSERT() does not build on those platforms is that
static_assert only works with compile-time constants, but mac uses libstdc++4.2
and android uses stlport, which both don't mark numeric_limits::max() as
constexpr.
R=Nico
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239352
Patch Set 1 #Patch Set 2 : Disable for Mac / Android / IOS instead of clang #
Total comments: 2
Patch Set 3 : Update comment and add cast #
Created: 7 years ago
Messages
Total messages: 12 (0 generated)
This looks wrong. It's failing with clang because we use libstdc++ 4.2 with it on mac, and numeric_limits::max() isn't marked constexpr there. Is it not constexpr in libstdc++4.7 either?
On 2013/12/06 21:23:35, Nico wrote:
> This looks wrong. It's failing with clang because we use libstdc++ 4.2 with it
> on mac, and numeric_limits::max() isn't marked constexpr there. Is it not
> constexpr in libstdc++4.7 either?
Here is the error I get:
FAILED:
/home/chris/devel/internal-trunk/trunk/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.7/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++
-MMD -MF obj/base/strings/base.safe_sprintf.o.d -DANGLE_DX11
-D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD
-DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DUSE_PROPRIETARY_CODECS
-DENABLE_CONFIGURATION_POLICY -DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DUSE_OPENSSL=1 -DENABLE_EGLIMAGE=1
-DENABLE_AUTOFILL_DIALOG=1 -DCLD_VERSION=1 -DENABLE_PRINTING=1
-DENABLE_MANAGED_USERS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-DBASE_IMPLEMENTATION -DANDROID -D__GNU_SOURCE=1 -DUSE_STLPORT=1
-D_STLP_USE_PTR_SPECIALIZATIONS=1 '-DCHROME_BUILD_ID=""' -DHAVE_SYS_UIO_H
-DNDEBUG -DOFFICIAL_BUILD -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
-D_FORTIFY_SOURCE=2 -Igen/base
-I../../third_party/android_tools/ndk/sources/android/cpufeatures -I../..
-fstack-protector --param=ssp-buffer-size=4 -Werror -fno-exceptions
-fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers
-fvisibility=hidden -pipe -fPIC -march=armv7-a -mtune=cortex-a8 -mfpu=vfpv3-d16
-mfloat-abi=softfp -mthumb -fno-tree-sra -fuse-ld=gold -Wno-psabi
-mthumb-interwork -ffunction-sections -funwind-tables -g -fstack-protector
-fno-short-enums -finline-limit=64 -Wa,--noexecstack
--sysroot=/home/chris/devel/internal-trunk/trunk/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-I/home/chris/devel/internal-trunk/trunk/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/stlport
-Os -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer
-std=gnu++11 -Wno-sign-compare -fno-rtti -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wno-abi -c ../../base/strings/safe_sprintf.cc -o
obj/base/strings/base.safe_sprintf.o
../../base/strings/safe_sprintf.cc: In constructor
'base::strings::{anonymous}::Buffer::Buffer(char*, size_t)':
../../base/strings/safe_sprintf.cc:115:5: error: call to non-constexpr function
'static _Int std::priv::_Integer_limits<_Int, __imin, __imax, __idigits,
__ismod>::max() [with _Int = long int; _Int __imin = -2147483648l; _Int __imax =
2147483647l; int __idigits = -1; bool __ismod = true]'
../../base/strings/safe_sprintf.cc:115:5: error: call to non-constexpr function
'static _Int std::priv::_Integer_limits<_Int, __imin, __imax, __idigits,
__ismod>::max() [with _Int = long int; _Int __imin = -2147483648l; _Int __imax =
2147483647l; int __idigits = -1; bool __ismod = true]'
../../base/strings/safe_sprintf.cc:115:5: note: in template argument for type
'bool'
../../base/strings/safe_sprintf.cc:116:61: error: invalid type in declaration
before ';' token
[1393/7919] CXX obj/base/strings/base.string16.o
ninja: build stopped: subcommand failed.
This is with arm-linux-androideabi-4.7 toolchain of Android NDK. No issue with
gcc 4.7 on Linux desktop though.
Oh, that's probably because Android builds with stlport which also doesn't have a constexpr max(). So !defined(OS_MAC) && !defined(OS_ANDROID) plus a comment that explains this is probably what you want.
On 2013/12/06 21:57:11, Nico wrote: > Oh, that's probably because Android builds with stlport which also doesn't have > a constexpr max(). So !defined(OS_MAC) && !defined(OS_ANDROID) plus a comment > that explains this is probably what you want. Done.
Can you please update the comment with the actual reason, instead of "something doesn't work"? Also in the CL description. (the reason is that static_assert only works with compile-time constants, but mac uses libstdc++4.2 and android uses stlport, which both don't mark numeric_limits::max() as constexpr)
https://codereview.chromium.org/98403005/diff/20001/base/strings/safe_sprintf.cc File base/strings/safe_sprintf.cc (right): https://codereview.chromium.org/98403005/diff/20001/base/strings/safe_sprintf... base/strings/safe_sprintf.cc:115: COMPILE_ASSERT(kSSizeMaxConst == std::numeric_limits<ssize_t>::max(), Note that on Linux desktop, gcc does complain about comparison between signed and unsigned integers though :(
https://codereview.chromium.org/98403005/diff/20001/base/strings/safe_sprintf.cc File base/strings/safe_sprintf.cc (right): https://codereview.chromium.org/98403005/diff/20001/base/strings/safe_sprintf... base/strings/safe_sprintf.cc:115: COMPILE_ASSERT(kSSizeMaxConst == std::numeric_limits<ssize_t>::max(), On 2013/12/06 22:18:10, Chris Dumez wrote: > Note that on Linux desktop, gcc does complain about comparison between signed > and unsigned integers though :( It should be safe to cast std::numeric_limits<ssize_t>::max() to size_t, no?
On 2013/12/06 22:21:45, Nico wrote: > https://codereview.chromium.org/98403005/diff/20001/base/strings/safe_sprintf.cc > File base/strings/safe_sprintf.cc (right): > > https://codereview.chromium.org/98403005/diff/20001/base/strings/safe_sprintf... > base/strings/safe_sprintf.cc:115: COMPILE_ASSERT(kSSizeMaxConst == > std::numeric_limits<ssize_t>::max(), > On 2013/12/06 22:18:10, Chris Dumez wrote: > > Note that on Linux desktop, gcc does complain about comparison between signed > > and unsigned integers though :( > > It should be safe to cast std::numeric_limits<ssize_t>::max() to size_t, no? I believe so since we know the value is not negative. It seems to build fine also with gcc. I updated the CL accordingly.
lgtm if the trybots are happy :-)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/98403005/40001
Message was sent while issue was closed.
Change committed as 239352 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
