Inconsistent check of COMPILER_GCC in base/compiler_specific.h |
||
Issue descriptionbuild/build_config.h defines one of two macros: https://cs.chromium.org/chromium/src/build/build_config.h // Compiler detection. #if defined(__GNUC__) #define COMPILER_GCC 1 #elif defined(_MSC_VER) #define COMPILER_MSVC 1 #else #error Please add support for your compiler in build/build_config.h #endif And in the most of the case, the following checks are performed: #if defined(COMPILER_GCC) // Do something. #elif defined(COMPILER_MSVC) // Do something else. #else // Do something completely else. #endif There's one use in base/compiler_specific.h that violates this convention, as it tests: https://cs.chromium.org/chromium/src/base/compiler_specific.h?q=compiler_specific.&sq=package:chromium&g=0&l=80 #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 That could lead to a preprocessor error on Windows. While this error is not triggered in Chromium code base, I was hit by it, while using WebRTC Native Code on Windows: <...> In file included from ../..\base/strings/stringprintf.h:13: ../..\base/compiler_specific.h(80,5): error: 'COMPILER_GCC' is not defined, evaluates to 0 [-Werror,-Wundef] #if COMPILER_GCC && defined(NDEBUG) ^ 1 error generated. It's interesting that "if COMPILER_GCC" has a single use across the whole Chromium codebase: https://cs.chromium.org/search/?q=if.COMPILER_GCC&sq=package:chromium&type=cs While "if defined(COMPILER_GCC)" has 25 uses: https://cs.chromium.org/search/?q=if.defined.COMPILER_GCC&sq=package:chromium&type=cs I propose to fix this discrepancy.
,
Aug 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9c098a0d2e3ab4b0f36581e15b820c6c232aa4da commit 9c098a0d2e3ab4b0f36581e15b820c6c232aa4da Author: Ivan Krasin <krasin@chromium.org> Date: Sun Aug 05 03:57:57 2018 Do not throw a preprocessor error, if COMPILER_GCC is not defined. build/build_config.h defines one of two macros: https://cs.chromium.org/chromium/src/build/build_config.h // Compiler detection. #if defined(__GNUC__) #define COMPILER_GCC 1 #elif defined(_MSC_VER) #define COMPILER_MSVC 1 #else #error Please add support for your compiler in build/build_config.h #endif And in the most of the case, the following checks are performed: #if defined(COMPILER_GCC) // Do something. #elif defined(COMPILER_MSVC) // Do something else. #else // Do something completely else. #endif There's one use in base/compiler_specific.h that violates this convention, as it tests: #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 That leads to a preprocessor error on Windows. While this error is not triggered in Chromium, it can be reproduced, if link WebRTC Native Code with //base shipped as a part of their checkout. BUG= chromium:871039 Change-Id: I868b4a47a574fba48e7d2743cd64ace99ea64940 Reviewed-on: https://chromium-review.googlesource.com/1162343 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Ivan Krasin <krasin@chromium.org> Cr-Commit-Position: refs/heads/master@{#580770} [modify] https://crrev.com/9c098a0d2e3ab4b0f36581e15b820c6c232aa4da/base/compiler_specific.h
,
Aug 5
|
||
►
Sign in to add a comment |
||
Comment 1 by krasin@chromium.org
, Aug 4