Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
847003004:
Linux: Properly detect __GLIBCXX__ in atomicops.h (Closed)
DescriptionLinux: Properly detect __GLIBCXX__ in atomicops.h
BUG=449754
Committed: https://crrev.com/2935666ed30c24a22c409351923d40c3586d2d3b
Cr-Commit-Position: refs/heads/master@{#312309}
Patch Set 1 #Patch Set 2 : fix sanitizer #
Total comments: 11
Patch Set 3 : cstddef #
Total comments: 2
Patch Set 4 : comment #Messages
Total messages: 22 (2 generated)
thestig@chromium.org changed reviewers: + earthdok@chromium.org, jfb@chromium.org, thakis@chromium.org
https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) Or maybe only include this if _LIBCPP_VERSION is not defined, and drop USE_CUSTOM_LIBCXX?
https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) Hm, this isn't great. We're going to ship non-custom-libc++s eventually. Maybe you can include iosfwd, this is hopefully small and might be enough to set _GLIBCXX__? (also not great, but at least it doesn't need this explicit define)
https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (left): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#oldcode33 base/atomicops.h:33: #include <stdint.h> Why change the order? https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) On 2015/01/21 00:03:31, Nico wrote: > Hm, this isn't great. We're going to ship non-custom-libc++s eventually. > > Maybe you can include iosfwd, this is hopefully small and might be enough to set > _GLIBCXX__? (also not great, but at least it doesn't need this explicit define) Is this just for LLVM? Or at least GCC 5.0? If so then use: #ifdef __has_include # if __has_include(bits/c++config.h)
https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) On 2015/01/21 00:05:08, JF wrote: > On 2015/01/21 00:03:31, Nico wrote: > > Hm, this isn't great. We're going to ship non-custom-libc++s eventually. > > > > Maybe you can include iosfwd, this is hopefully small and might be enough to > set > > _GLIBCXX__? (also not great, but at least it doesn't need this explicit > define) > > Is this just for LLVM? Or at least GCC 5.0? If so then use: > > #ifdef __has_include > # if __has_include(bits/c++config.h) No, we use libc++ with gcc, and we don't require gcc 5.0 (and probably won't for a long time)
https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) On 2015/01/21 00:06:04, Nico wrote: > On 2015/01/21 00:05:08, JF wrote: > > On 2015/01/21 00:03:31, Nico wrote: > > > Hm, this isn't great. We're going to ship non-custom-libc++s eventually. > > > > > > Maybe you can include iosfwd, this is hopefully small and might be enough to > > set > > > _GLIBCXX__? (also not great, but at least it doesn't need this explicit > > define) > > > > Is this just for LLVM? Or at least GCC 5.0? If so then use: > > > > #ifdef __has_include > > # if __has_include(bits/c++config.h) > > No, we use libc++ with gcc, and we don't require gcc 5.0 (and probably won't for > a long time) _LIBCPP_VERSION should be what we use to detect libc++ then, and _GLIBCXX_VERSION for libstdc++.
https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) On 2015/01/21 00:15:22, JF wrote: > On 2015/01/21 00:06:04, Nico wrote: > > On 2015/01/21 00:05:08, JF wrote: > > > On 2015/01/21 00:03:31, Nico wrote: > > > > Hm, this isn't great. We're going to ship non-custom-libc++s eventually. > > > > > > > > Maybe you can include iosfwd, this is hopefully small and might be enough > to > > > set > > > > _GLIBCXX__? (also not great, but at least it doesn't need this explicit > > > define) > > > > > > Is this just for LLVM? Or at least GCC 5.0? If so then use: > > > > > > #ifdef __has_include > > > # if __has_include(bits/c++config.h) > > > > No, we use libc++ with gcc, and we don't require gcc 5.0 (and probably won't > for > > a long time) > > _LIBCPP_VERSION should be what we use to detect libc++ then, and > _GLIBCXX_VERSION for libstdc++. Yah, but the problem is (as far as I understand) that these are set in the library headers, so we need to include some c++ library header here, else neither macro will be defined. (cassert doesn't qualify since it's really just a c header.)
https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (left): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#oldcode33 base/atomicops.h:33: #include <stdint.h> On 2015/01/21 00:05:08, JF wrote: > Why change the order? C headers before C++ headers https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) On 2015/01/21 00:16:58, Nico wrote: > On 2015/01/21 00:15:22, JF wrote: > > On 2015/01/21 00:06:04, Nico wrote: > > > On 2015/01/21 00:05:08, JF wrote: > > > > On 2015/01/21 00:03:31, Nico wrote: > > > > > Hm, this isn't great. We're going to ship non-custom-libc++s eventually. > > > > > > > > > > Maybe you can include iosfwd, this is hopefully small and might be > enough > > to > > > > set > > > > > _GLIBCXX__? (also not great, but at least it doesn't need this explicit > > > > define) > > > > > > > > Is this just for LLVM? Or at least GCC 5.0? If so then use: > > > > > > > > #ifdef __has_include > > > > # if __has_include(bits/c++config.h) > > > > > > No, we use libc++ with gcc, and we don't require gcc 5.0 (and probably won't > > for > > > a long time) > > > > _LIBCPP_VERSION should be what we use to detect libc++ then, and > > _GLIBCXX_VERSION for libstdc++. > > Yah, but the problem is (as far as I understand) that these are set in the > library headers, so we need to include some c++ library header here, else > neither macro will be defined. (cassert doesn't qualify since it's really just a > c header.) cassert in libc++ brings in _LIBCPP_VERSION, but before this CL, nothing is bringing in __GLIBCXX__ for libstdc++, so I went straight to the source.
https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) On 2015/01/21 00:19:20, Lei Zhang wrote: > On 2015/01/21 00:16:58, Nico wrote: > > On 2015/01/21 00:15:22, JF wrote: > > > On 2015/01/21 00:06:04, Nico wrote: > > > > On 2015/01/21 00:05:08, JF wrote: > > > > > On 2015/01/21 00:03:31, Nico wrote: > > > > > > Hm, this isn't great. We're going to ship non-custom-libc++s > eventually. > > > > > > > > > > > > Maybe you can include iosfwd, this is hopefully small and might be > > enough > > > to > > > > > set > > > > > > _GLIBCXX__? (also not great, but at least it doesn't need this > explicit > > > > > define) > > > > > > > > > > Is this just for LLVM? Or at least GCC 5.0? If so then use: > > > > > > > > > > #ifdef __has_include > > > > > # if __has_include(bits/c++config.h) > > > > > > > > No, we use libc++ with gcc, and we don't require gcc 5.0 (and probably > won't > > > for > > > > a long time) > > > > > > _LIBCPP_VERSION should be what we use to detect libc++ then, and > > > _GLIBCXX_VERSION for libstdc++. > > > > Yah, but the problem is (as far as I understand) that these are set in the > > library headers, so we need to include some c++ library header here, else > > neither macro will be defined. (cassert doesn't qualify since it's really just > a > > c header.) > > cassert in libc++ brings in _LIBCPP_VERSION, but before this CL, nothing is > bringing in __GLIBCXX__ for libstdc++, so I went straight to the source. Right, but iosfwd will probably bring in a config file for all c++ libraries, no? Then you don't need to know the c++ library to decide which header to include to get access to a macro that enables you to know which c++ library you're using.
https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) On 2015/01/21 00:24:56, Nico wrote: > On 2015/01/21 00:19:20, Lei Zhang wrote: > > On 2015/01/21 00:16:58, Nico wrote: > > > On 2015/01/21 00:15:22, JF wrote: > > > > On 2015/01/21 00:06:04, Nico wrote: > > > > > On 2015/01/21 00:05:08, JF wrote: > > > > > > On 2015/01/21 00:03:31, Nico wrote: > > > > > > > Hm, this isn't great. We're going to ship non-custom-libc++s > > eventually. > > > > > > > > > > > > > > Maybe you can include iosfwd, this is hopefully small and might be > > > enough > > > > to > > > > > > set > > > > > > > _GLIBCXX__? (also not great, but at least it doesn't need this > > explicit > > > > > > define) > > > > > > > > > > > > Is this just for LLVM? Or at least GCC 5.0? If so then use: > > > > > > > > > > > > #ifdef __has_include > > > > > > # if __has_include(bits/c++config.h) > > > > > > > > > > No, we use libc++ with gcc, and we don't require gcc 5.0 (and probably > > won't > > > > for > > > > > a long time) > > > > > > > > _LIBCPP_VERSION should be what we use to detect libc++ then, and > > > > _GLIBCXX_VERSION for libstdc++. > > > > > > Yah, but the problem is (as far as I understand) that these are set in the > > > library headers, so we need to include some c++ library header here, else > > > neither macro will be defined. (cassert doesn't qualify since it's really > just > > a > > > c header.) > > > > cassert in libc++ brings in _LIBCPP_VERSION, but before this CL, nothing is > > bringing in __GLIBCXX__ for libstdc++, so I went straight to the source. > > Right, but iosfwd will probably bring in a config file for all c++ libraries, > no? Then you don't need to know the c++ library to decide which header to > include to get access to a macro that enables you to know which c++ library > you're using. Right I was seeing this the wrong way around. Here's what brings in bits/c++config.h in libstdc++: -rw-r----- 1 jfb eng 68K Sep 6 2013 include/std/functional -rw-r----- 1 jfb eng 60K Sep 6 2013 include/std/limits -rw-r----- 1 jfb eng 52K Sep 6 2013 include/std/complex -rw-r----- 1 jfb eng 38K Sep 6 2013 include/std/valarray -rw-r----- 1 jfb eng 36K Sep 6 2013 include/std/type_traits -rw-r----- 1 jfb eng 28K Sep 6 2013 include/std/streambuf -rw-r----- 1 jfb eng 11K Sep 6 2013 include/std/iomanip -rw-r----- 1 jfb eng 9.8K Sep 6 2013 include/std/system_error -rw-r----- 1 jfb eng 6.8K Aug 2 2013 include/std/iosfwd -rw-r----- 1 jfb eng 4.7K Sep 6 2013 include/std/utility -rw-r----- 1 jfb eng 2.8K Aug 2 2013 include/std/numeric -rw-r----- 1 jfb eng 2.7K Aug 2 2013 include/std/iostream -rw-r----- 1 jfb eng 2.6K Aug 2 2013 include/std/iterator -rw-r----- 1 jfb eng 2.0K Aug 2 2013 include/std/string Here's what brings in __config in libc++: -rw-r----- 1 jfb eng 221K Mar 22 2014 include/random -rw-r----- 1 jfb eng 215K Aug 21 15:09 include/regex -rw-r----- 1 jfb eng 196K Aug 21 15:09 include/algorithm -rw-r----- 1 jfb eng 169K Aug 21 15:09 include/memory -rw-r----- 1 jfb eng 152K Aug 21 15:09 include/string -rw-r----- 1 jfb eng 151K Aug 21 15:09 include/locale -rw-r----- 1 jfb eng 131K Aug 21 15:09 include/valarray -rw-r----- 1 jfb eng 108K Aug 21 15:09 include/type_traits -rw-r----- 1 jfb eng 107K Aug 21 15:09 include/vector -rw-r----- 1 jfb eng 101K Aug 21 15:09 include/deque -rw-r----- 1 jfb eng 81K Aug 21 15:09 include/unordered_map -rw-r----- 1 jfb eng 76K Aug 21 15:09 include/list -rw-r----- 1 jfb eng 75K Aug 21 15:09 include/map -rw-r----- 1 jfb eng 70K Aug 21 15:09 include/future -rw-r----- 1 jfb eng 69K Aug 21 15:09 include/functional -rw-r----- 1 jfb eng 59K Aug 21 15:09 include/atomic -rw-r----- 1 jfb eng 56K Aug 21 15:09 include/forward_list -rw-r----- 1 jfb eng 54K Aug 21 15:09 include/unordered_set -rw-r----- 1 jfb eng 52K Aug 21 15:09 include/iterator -rw-r----- 1 jfb eng 49K Mar 22 2014 include/istream -rw-r----- 1 jfb eng 46K Aug 21 15:09 include/cmath -rw-r----- 1 jfb eng 45K Aug 21 15:09 include/set -rw-r----- 1 jfb eng 44K Mar 22 2014 include/complex -rw-r----- 1 jfb eng 43K Mar 22 2014 include/fstream -rw-r----- 1 jfb eng 41K Aug 21 15:09 include/tuple -rw-r----- 1 jfb eng 40K Aug 21 15:09 include/limits -rw-r----- 1 jfb eng 36K Mar 22 2014 include/chrono -rw-r----- 1 jfb eng 34K Aug 21 15:09 include/bitset -rw-r----- 1 jfb eng 33K Mar 22 2014 include/sstream -rw-r----- 1 jfb eng 32K Aug 21 15:09 include/ostream -rw-r----- 1 jfb eng 25K Mar 22 2014 include/queue -rw-r----- 1 jfb eng 25K Mar 22 2014 include/ios -rw-r----- 1 jfb eng 24K Aug 21 15:09 include/utility -rw-r----- 1 jfb eng 22K Mar 22 2014 include/system_error -rw-r----- 1 jfb eng 22K Mar 22 2014 include/scoped_allocator -rw-r----- 1 jfb eng 20K Mar 22 2014 include/codecvt -rw-r----- 1 jfb eng 18K Aug 21 15:09 include/iomanip -rw-r----- 1 jfb eng 16K Mar 22 2014 include/streambuf -rw-r----- 1 jfb eng 15K Mar 22 2014 include/ratio -rw-r----- 1 jfb eng 14K Aug 21 15:09 include/mutex -rw-r----- 1 jfb eng 12K Aug 21 15:09 include/shared_mutex -rw-r----- 1 jfb eng 12K Mar 22 2014 include/thread -rw-r----- 1 jfb eng 12K Aug 29 2013 include/strstream -rw-r----- 1 jfb eng 11K Mar 22 2014 include/array -rw-r----- 1 jfb eng 9.0K Mar 22 2014 include/stack -rw-r----- 1 jfb eng 7.6K Mar 22 2014 include/cwchar -rw-r----- 1 jfb eng 7.5K Mar 22 2014 include/iosfwd -rw-r----- 1 jfb eng 7.3K Mar 22 2014 include/exception -rw-r----- 1 jfb eng 7.2K Aug 29 2013 include/condition_variable -rw-r----- 1 jfb eng 6.2K Mar 22 2014 include/numeric -rw-r----- 1 jfb eng 6.1K Aug 29 2013 include/cwctype -rw-r----- 1 jfb eng 5.3K Mar 22 2014 include/cstdlib -rw-r----- 1 jfb eng 5.0K Aug 21 15:09 include/new -rw-r----- 1 jfb eng 5.0K Aug 29 2013 include/cerrno -rw-r----- 1 jfb eng 4.8K Mar 22 2014 include/cstdio -rw-r----- 1 jfb eng 4.6K Mar 22 2014 include/cctype -rw-r----- 1 jfb eng 4.3K Aug 21 15:09 include/stdexcept -rw-r----- 1 jfb eng 4.3K Aug 21 15:09 include/typeinfo -rw-r----- 1 jfb eng 3.5K Aug 29 2013 include/cinttypes -rw-r----- 1 jfb eng 3.3K Mar 22 2014 include/cstring -rw-r----- 1 jfb eng 2.8K Mar 22 2014 include/initializer_list -rw-r----- 1 jfb eng 2.8K Mar 22 2014 include/typeindex -rw-r----- 1 jfb eng 2.8K Aug 29 2013 include/cstdint -rw-r----- 1 jfb eng 2.6K Aug 21 15:09 include/cstddef -rw-r----- 1 jfb eng 1.6K Aug 29 2013 include/cfenv -rw-r----- 1 jfb eng 1.4K Aug 29 2013 include/ctime -rw-r----- 1 jfb eng 1.3K Aug 29 2013 include/iostream -rw-r----- 1 jfb eng 1.3K Aug 29 2013 include/cfloat -rw-r----- 1 jfb eng 951 Aug 29 2013 include/csignal -rw-r----- 1 jfb eng 929 Aug 29 2013 include/clocale -rw-r----- 1 jfb eng 894 Aug 29 2013 include/climits -rw-r----- 1 jfb eng 890 Aug 29 2013 include/cstdarg -rw-r----- 1 jfb eng 855 Aug 29 2013 include/csetjmp -rw-r----- 1 jfb eng 708 Aug 29 2013 include/cstdbool -rw-r----- 1 jfb eng 582 Aug 29 2013 include/ciso646 -rw-r----- 1 jfb eng 546 Aug 29 2013 include/cassert How about we go with iostream?
On 2015/01/21 00:34:02, JF wrote: > https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h > File base/atomicops.h (right): > > https://codereview.chromium.org/847003004/diff/20001/base/atomicops.h#newcode39 > base/atomicops.h:39: #if defined(OS_LINUX) && !defined(USE_CUSTOM_LIBCXX) > On 2015/01/21 00:24:56, Nico wrote: > > On 2015/01/21 00:19:20, Lei Zhang wrote: > > > On 2015/01/21 00:16:58, Nico wrote: > > > > On 2015/01/21 00:15:22, JF wrote: > > > > > On 2015/01/21 00:06:04, Nico wrote: > > > > > > On 2015/01/21 00:05:08, JF wrote: > > > > > > > On 2015/01/21 00:03:31, Nico wrote: > > > > > > > > Hm, this isn't great. We're going to ship non-custom-libc++s > > > eventually. > > > > > > > > > > > > > > > > Maybe you can include iosfwd, this is hopefully small and might be > > > > enough > > > > > to > > > > > > > set > > > > > > > > _GLIBCXX__? (also not great, but at least it doesn't need this > > > explicit > > > > > > > define) > > > > > > > > > > > > > > Is this just for LLVM? Or at least GCC 5.0? If so then use: > > > > > > > > > > > > > > #ifdef __has_include > > > > > > > # if __has_include(bits/c++config.h) > > > > > > > > > > > > No, we use libc++ with gcc, and we don't require gcc 5.0 (and probably > > > won't > > > > > for > > > > > > a long time) > > > > > > > > > > _LIBCPP_VERSION should be what we use to detect libc++ then, and > > > > > _GLIBCXX_VERSION for libstdc++. > > > > > > > > Yah, but the problem is (as far as I understand) that these are set in the > > > > library headers, so we need to include some c++ library header here, else > > > > neither macro will be defined. (cassert doesn't qualify since it's really > > just > > > a > > > > c header.) > > > > > > cassert in libc++ brings in _LIBCPP_VERSION, but before this CL, nothing is > > > bringing in __GLIBCXX__ for libstdc++, so I went straight to the source. > > > > Right, but iosfwd will probably bring in a config file for all c++ libraries, > > no? Then you don't need to know the c++ library to decide which header to > > include to get access to a macro that enables you to know which c++ library > > you're using. > > Right I was seeing this the wrong way around. > > Here's what brings in bits/c++config.h in libstdc++: > -rw-r----- 1 jfb eng 68K Sep 6 2013 include/std/functional > -rw-r----- 1 jfb eng 60K Sep 6 2013 include/std/limits > -rw-r----- 1 jfb eng 52K Sep 6 2013 include/std/complex > -rw-r----- 1 jfb eng 38K Sep 6 2013 include/std/valarray > -rw-r----- 1 jfb eng 36K Sep 6 2013 include/std/type_traits > -rw-r----- 1 jfb eng 28K Sep 6 2013 include/std/streambuf > -rw-r----- 1 jfb eng 11K Sep 6 2013 include/std/iomanip > -rw-r----- 1 jfb eng 9.8K Sep 6 2013 include/std/system_error > -rw-r----- 1 jfb eng 6.8K Aug 2 2013 include/std/iosfwd > -rw-r----- 1 jfb eng 4.7K Sep 6 2013 include/std/utility > -rw-r----- 1 jfb eng 2.8K Aug 2 2013 include/std/numeric > -rw-r----- 1 jfb eng 2.7K Aug 2 2013 include/std/iostream > -rw-r----- 1 jfb eng 2.6K Aug 2 2013 include/std/iterator > -rw-r----- 1 jfb eng 2.0K Aug 2 2013 include/std/string > > Here's what brings in __config in libc++: > -rw-r----- 1 jfb eng 221K Mar 22 2014 include/random > -rw-r----- 1 jfb eng 215K Aug 21 15:09 include/regex > -rw-r----- 1 jfb eng 196K Aug 21 15:09 include/algorithm > -rw-r----- 1 jfb eng 169K Aug 21 15:09 include/memory > -rw-r----- 1 jfb eng 152K Aug 21 15:09 include/string > -rw-r----- 1 jfb eng 151K Aug 21 15:09 include/locale > -rw-r----- 1 jfb eng 131K Aug 21 15:09 include/valarray > -rw-r----- 1 jfb eng 108K Aug 21 15:09 include/type_traits > -rw-r----- 1 jfb eng 107K Aug 21 15:09 include/vector > -rw-r----- 1 jfb eng 101K Aug 21 15:09 include/deque > -rw-r----- 1 jfb eng 81K Aug 21 15:09 include/unordered_map > -rw-r----- 1 jfb eng 76K Aug 21 15:09 include/list > -rw-r----- 1 jfb eng 75K Aug 21 15:09 include/map > -rw-r----- 1 jfb eng 70K Aug 21 15:09 include/future > -rw-r----- 1 jfb eng 69K Aug 21 15:09 include/functional > -rw-r----- 1 jfb eng 59K Aug 21 15:09 include/atomic > -rw-r----- 1 jfb eng 56K Aug 21 15:09 include/forward_list > -rw-r----- 1 jfb eng 54K Aug 21 15:09 include/unordered_set > -rw-r----- 1 jfb eng 52K Aug 21 15:09 include/iterator > -rw-r----- 1 jfb eng 49K Mar 22 2014 include/istream > -rw-r----- 1 jfb eng 46K Aug 21 15:09 include/cmath > -rw-r----- 1 jfb eng 45K Aug 21 15:09 include/set > -rw-r----- 1 jfb eng 44K Mar 22 2014 include/complex > -rw-r----- 1 jfb eng 43K Mar 22 2014 include/fstream > -rw-r----- 1 jfb eng 41K Aug 21 15:09 include/tuple > -rw-r----- 1 jfb eng 40K Aug 21 15:09 include/limits > -rw-r----- 1 jfb eng 36K Mar 22 2014 include/chrono > -rw-r----- 1 jfb eng 34K Aug 21 15:09 include/bitset > -rw-r----- 1 jfb eng 33K Mar 22 2014 include/sstream > -rw-r----- 1 jfb eng 32K Aug 21 15:09 include/ostream > -rw-r----- 1 jfb eng 25K Mar 22 2014 include/queue > -rw-r----- 1 jfb eng 25K Mar 22 2014 include/ios > -rw-r----- 1 jfb eng 24K Aug 21 15:09 include/utility > -rw-r----- 1 jfb eng 22K Mar 22 2014 include/system_error > -rw-r----- 1 jfb eng 22K Mar 22 2014 include/scoped_allocator > -rw-r----- 1 jfb eng 20K Mar 22 2014 include/codecvt > -rw-r----- 1 jfb eng 18K Aug 21 15:09 include/iomanip > -rw-r----- 1 jfb eng 16K Mar 22 2014 include/streambuf > -rw-r----- 1 jfb eng 15K Mar 22 2014 include/ratio > -rw-r----- 1 jfb eng 14K Aug 21 15:09 include/mutex > -rw-r----- 1 jfb eng 12K Aug 21 15:09 include/shared_mutex > -rw-r----- 1 jfb eng 12K Mar 22 2014 include/thread > -rw-r----- 1 jfb eng 12K Aug 29 2013 include/strstream > -rw-r----- 1 jfb eng 11K Mar 22 2014 include/array > -rw-r----- 1 jfb eng 9.0K Mar 22 2014 include/stack > -rw-r----- 1 jfb eng 7.6K Mar 22 2014 include/cwchar > -rw-r----- 1 jfb eng 7.5K Mar 22 2014 include/iosfwd > -rw-r----- 1 jfb eng 7.3K Mar 22 2014 include/exception > -rw-r----- 1 jfb eng 7.2K Aug 29 2013 include/condition_variable > -rw-r----- 1 jfb eng 6.2K Mar 22 2014 include/numeric > -rw-r----- 1 jfb eng 6.1K Aug 29 2013 include/cwctype > -rw-r----- 1 jfb eng 5.3K Mar 22 2014 include/cstdlib > -rw-r----- 1 jfb eng 5.0K Aug 21 15:09 include/new > -rw-r----- 1 jfb eng 5.0K Aug 29 2013 include/cerrno > -rw-r----- 1 jfb eng 4.8K Mar 22 2014 include/cstdio > -rw-r----- 1 jfb eng 4.6K Mar 22 2014 include/cctype > -rw-r----- 1 jfb eng 4.3K Aug 21 15:09 include/stdexcept > -rw-r----- 1 jfb eng 4.3K Aug 21 15:09 include/typeinfo > -rw-r----- 1 jfb eng 3.5K Aug 29 2013 include/cinttypes > -rw-r----- 1 jfb eng 3.3K Mar 22 2014 include/cstring > -rw-r----- 1 jfb eng 2.8K Mar 22 2014 include/initializer_list > -rw-r----- 1 jfb eng 2.8K Mar 22 2014 include/typeindex > -rw-r----- 1 jfb eng 2.8K Aug 29 2013 include/cstdint > -rw-r----- 1 jfb eng 2.6K Aug 21 15:09 include/cstddef > -rw-r----- 1 jfb eng 1.6K Aug 29 2013 include/cfenv > -rw-r----- 1 jfb eng 1.4K Aug 29 2013 include/ctime > -rw-r----- 1 jfb eng 1.3K Aug 29 2013 include/iostream > -rw-r----- 1 jfb eng 1.3K Aug 29 2013 include/cfloat > -rw-r----- 1 jfb eng 951 Aug 29 2013 include/csignal > -rw-r----- 1 jfb eng 929 Aug 29 2013 include/clocale > -rw-r----- 1 jfb eng 894 Aug 29 2013 include/climits > -rw-r----- 1 jfb eng 890 Aug 29 2013 include/cstdarg > -rw-r----- 1 jfb eng 855 Aug 29 2013 include/csetjmp > -rw-r----- 1 jfb eng 708 Aug 29 2013 include/cstdbool > -rw-r----- 1 jfb eng 582 Aug 29 2013 include/ciso646 > -rw-r----- 1 jfb eng 546 Aug 29 2013 include/cassert > > > > > How about we go with iostream? Actually that's a horrible suggestion because it has a bunch of other includes...
Patch set 3 uses cstddef, which brings in __config in libc++ and bits/c++config.h for libstdc++.
On 2015/01/21 00:38:52, Lei Zhang wrote: > Patch set 3 uses cstddef, which brings in __config in libc++ and > bits/c++config.h for libstdc++. Are you sure for libstdc++? It's not in jf's list above, and I'd be surprised if cstddef brought it in but cassert doesn't. If you're sure, lgtm.
On 2015/01/21 00:40:59, Nico wrote: > On 2015/01/21 00:38:52, Lei Zhang wrote: > > Patch set 3 uses cstddef, which brings in __config in libc++ and > > bits/c++config.h for libstdc++. > > Are you sure for libstdc++? It's not in jf's list above, and I'd be surprised if > cstddef brought it in but cassert doesn't. > > If you're sure, lgtm. Yes, I looked in /usr/include/c++/4.8/cstddef and /usr/include/c++/4.8/cassert on a Linux box. The 4.6 version in a chroot looks the same, except for the copyright dates.
jf, where did you get your libstdc++ list from? On Tue, Jan 20, 2015 at 4:45 PM, <thestig@chromium.org> wrote: > On 2015/01/21 00:40:59, Nico wrote: > >> On 2015/01/21 00:38:52, Lei Zhang wrote: >> > Patch set 3 uses cstddef, which brings in __config in libc++ and >> > bits/c++config.h for libstdc++. >> > > Are you sure for libstdc++? It's not in jf's list above, and I'd be >> surprised >> > if > >> cstddef brought it in but cassert doesn't. >> > > If you're sure, lgtm. >> > > Yes, I looked in /usr/include/c++/4.8/cstddef and > /usr/include/c++/4.8/cassert > on a Linux box. The 4.6 version in a chroot looks the same, except for the > copyright dates. > > https://codereview.chromium.org/847003004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/21 00:40:59, Nico wrote: > On 2015/01/21 00:38:52, Lei Zhang wrote: > > Patch set 3 uses cstddef, which brings in __config in libc++ and > > bits/c++config.h for libstdc++. > > Are you sure for libstdc++? It's not in jf's list above, and I'd be surprised if > cstddef brought it in but cassert doesn't. > > If you're sure, lgtm. My grep-fu was wrong, I pruned out too much. For libc++ here's the include count: git grep "include <__config>" ./include/* | grep -v "/__" | grep -v experimental | grep -v "/ext/" | cut -f1 -d: | xargs grep -Hc "#include" | sed 's/\(.*\):\(.*\)/\2 \1/g' | sort -h 1 include/ciso646 1 include/cstdbool 2 include/cassert 2 include/cerrno 2 include/cfenv 2 include/cfloat 2 include/climits 2 include/clocale 2 include/codecvt 2 include/csetjmp 2 include/csignal 2 include/cstdarg 2 include/cstddef 2 include/cstdint 2 include/cstring 2 include/ctime 2 include/initializer_list 2 include/iomanip 2 include/iosfwd 2 include/numeric 2 include/scoped_allocator 2 include/stack 2 include/type_traits 3 include/cctype 3 include/condition_variable 3 include/cstdio 3 include/cstdlib 3 include/cwctype 3 include/exception 3 include/istream 3 include/new 3 include/set 3 include/shared_mutex 3 include/stdexcept 3 include/streambuf 3 include/strstream 3 include/typeindex 3 include/utility 4 include/atomic 4 include/cinttypes 4 include/cmath 4 include/cwchar 4 include/typeinfo 5 include/limits 5 include/mutex 5 include/queue 5 include/ratio 5 include/sstream 5 include/system_error 5 include/unordered_set 6 include/chrono 6 include/complex 6 include/fstream 6 include/ios 6 include/ostream 6 include/tuple 6 include/unordered_map 7 include/forward_list 7 include/future 7 include/map 8 include/array 8 include/deque 8 include/functional 8 include/iterator 8 include/list 8 include/valarray 9 include/iostream 10 include/bitset 12 include/regex 12 include/thread 13 include/algorithm 14 include/vector 15 include/locale 15 include/memory 15 include/random 17 include/string For libstdc++: git grep c++config.h ./include/ | grep -v "\.h:"|grep -v Makefile | grep "std/" | cut -d: -f1 | xargs grep -Hc include | sed 's/\(.*\):\(.*\)/\2 \1/g' | sort -h 2 include/std/limits 3 include/c_std/clocale 3 include/c_std/csetjmp 3 include/c_std/csignal 3 include/c_std/cstddef 3 include/c_std/cstring 3 include/std/type_traits 4 include/c_std/cctype 4 include/c_std/cstdarg 4 include/c_std/cstdio 4 include/c_std/cstdlib 4 include/c_std/ctime 4 include/c_std/cwchar 4 include/std/iostream 5 include/c_std/cwctype 5 include/std/iomanip 5 include/std/iosfwd 5 include/std/numeric 6 include/c_std/cmath 6 include/std/complex 6 include/std/utility 7 include/std/system_error 8 include/std/streambuf 9 include/std/functional 10 include/std/iterator 15 include/std/valarray 17 include/std/string cstddef looks fine: In libc++: include/cstddef:#include <__config> include/cstddef:#include <stddef.h> In libstdc++: include/c_std/cstddef: * This is a Standard C++ Library file. You should @c #include this file include/c_std/cstddef:#include <bits/c++config.h> include/c_std/cstddef:#include <stddef.h> So lgtm.
https://codereview.chromium.org/847003004/diff/40001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/40001/base/atomicops.h#newcode34 base/atomicops.h:34: // macros used to identify the STL implementation. Actually, could you update the comment to say it captures __config in libc++ and bits/c++config.h in libstdc++, which gives access to _LIBCPP_VERSION and __GLIBCXX__ respectively.
https://codereview.chromium.org/847003004/diff/40001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/847003004/diff/40001/base/atomicops.h#newcode34 base/atomicops.h:34: // macros used to identify the STL implementation. On 2015/01/21 00:50:16, JF wrote: > Actually, could you update the comment to say it captures __config in libc++ and > bits/c++config.h in libstdc++, which gives access to _LIBCPP_VERSION and > __GLIBCXX__ respectively. Done.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/847003004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2935666ed30c24a22c409351923d40c3586d2d3b Cr-Commit-Position: refs/heads/master@{#312309} |
