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

Remove Acquire_Store() and Release_Load() functions #28

Closed
wants to merge 2 commits into from
Closed

Remove Acquire_Store() and Release_Load() functions #28

wants to merge 2 commits into from

Conversation

edmonds
Copy link
Contributor

@edmonds edmonds commented Sep 19, 2014

These functions are not used anywhere in protobuf. As a side effect, this fixes #25, since the buggy implementations are removed entirely.

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
These functions are not used anywhere in protobuf. As a side effect,
this fixes #25, since the buggy implementations are removed entirely.
@xfxyjwf
Copy link
Contributor

xfxyjwf commented Sep 19, 2014

Can you only remove the one in atomicops_internals_generic_gcc.h? Most of the atomics code is forked from v8/chromium. It's better to avoid local modifications to make future syncs easier.

@edmonds
Copy link
Contributor Author

edmonds commented Sep 19, 2014

Oh, I guess I misunderstood when you asked to have those two methods removed.

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.

Clang generates atomic memory ordering warnings when compiling atomicops_internals_generic_gcc.h
2 participants