Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix atomicops build failure on non-Clang #27

Merged
merged 1 commit into from
Sep 19, 2014
Merged

Fix atomicops build failure on non-Clang #27

merged 1 commit into from
Sep 19, 2014

Conversation

edmonds
Copy link
Contributor

@edmonds edmonds commented Sep 18, 2014

We cannot use Clang's __has_extension macro unless we really are compiling on Clang, which means we cannot use this expression:

    #if (defined(__clang__) && __has_extension(c_atomic)))
    // ...
    #endif

On GCC, this generates the following errors:

    In file included from ./google/protobuf/stubs/atomicops.h:59:0,
                     from google/protobuf/stubs/atomicops_internals_x86_gcc.cc:36:
    ./google/protobuf/stubs/platform_macros.h:67:41: error: missing binary operator before token "("
       (defined(__clang__) && __has_extension(c_atomic)))
                                             ^
    In file included from google/protobuf/stubs/atomicops_internals_x86_gcc.cc:36:0:
    ./google/protobuf/stubs/atomicops.h:196:40: error: missing binary operator before token "("
      (defined(__clang__) && __has_extension(c_atomic))
                                            ^

Instead, we have to protect the __has_extension expression by only executing it when __clang__ is defined:

    #if defined(__clang__)
    # if __has_extension(c_atomic)
    // ...
    # endif
    #endif

We cannot use Clang's __has_extension macro unless we really are
compiling on Clang, which means we cannot use this expression:

    #if (defined(__clang__) && __has_extension(c_atomic)))
    // ...
    #endif

On GCC, this generates the following errors:

    In file included from ./google/protobuf/stubs/atomicops.h:59:0,
                     from google/protobuf/stubs/atomicops_internals_x86_gcc.cc:36:
    ./google/protobuf/stubs/platform_macros.h:67:41: error: missing binary operator before token "("
       (defined(__clang__) && __has_extension(c_atomic)))
                                             ^
    In file included from google/protobuf/stubs/atomicops_internals_x86_gcc.cc:36:0:
    ./google/protobuf/stubs/atomicops.h:196:40: error: missing binary operator before token "("
      (defined(__clang__) && __has_extension(c_atomic))
                                            ^

Instead, we have to protect the __has_extension expression by only
executing it when __clang__ is defined:

    #if defined(__clang__)
    # if __has_extension(c_atomic)
    // ...
    # endif
    #endif
@edmonds
Copy link
Contributor Author

edmonds commented Sep 18, 2014

BTW, I've tested that this compiles on 8 different Debian platforms: {gcc,clang}/{amd64,i386,powerpc,ppc64el}.

@@ -33,6 +33,9 @@

#include <google/protobuf/stubs/common.h>

#define GOOGLE_PROTOBUF_PLATFORM_ERROR \
#error "Host platform was not detected as supported by protobuf"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this macro only contain the string literal? I remember "#define XXX #error" doesn't compile on some platforms (likely to be VC on windows).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the style from https://github.com/google/protobuf/blob/v2.6.0/src/google/protobuf/stubs/atomicops.h#L163, so there is already an instance of that construct in the code base :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm not quite sure what you're asking for. Do you want the end result to be a bare string constant, causing a syntax error, rather than using #error at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was suggesting:
#define PLATFORM_ERROR "Host platform ..."
and then
#error PLATFORM_ERROR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be ok if such "#define XXX #error" is already used elsewhere in protobuf. I'll merge the patch.

xfxyjwf added a commit that referenced this pull request Sep 19, 2014
…n_clang

Fix atomicops build failure on non-Clang
@xfxyjwf xfxyjwf merged commit 8d5f1e1 into protocolbuffers:master Sep 19, 2014
@edmonds edmonds deleted the branches/fix_generic_atomicops_non_clang branch September 19, 2014 00:48
TeBoring pushed a commit to TeBoring/protobuf that referenced this pull request Jan 19, 2019
Ported upb to C89, for greater portability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants