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

Workaround clang4.0 <stdatomic.h> initialization #trivial #426

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

bkase
Copy link
Contributor

@bkase bkase commented Jul 7, 2017

Texture fails to build with this error on clang 4.0:

external/Texture/pod_support/Headers/Public/AsyncDisplayKit/ASDispatch.h:32:35: error: illegal initializer type 'atomic_size_t' (aka '_Atomic(size_t)')
  __block atomic_size_t counter = ATOMIC_VAR_INIT(0);
                                  ^
In module 'std' imported from external/Texture/pod_support/Headers/Public/AsyncDisplayKit/ASStackUnpositionedLayout.h:18:
/private/var/tmp/_bazel_bkase/a00d4cbe29902fb63d5778cc19944cd2/external/clang40/bin/../include/c++/v1/atomic:1839:30: note: expanded from macro 'ATOMIC_VAR_INIT'
                             ^
1 error generated.

See
http://techqa.info/programming/question/38233019/Initializing-an--atomic-int--with-a-braced-constant--Is-this-valid-C-code--If-so-why-does-it-not-compile-in-clang-

Replacing the intialization with just 0 (and not the macro fixes the
error). The forum post notes:

C11's <stdatomic.h> header defines a macro ATOMIC_VAR_INIT that's intended to be used to initialize atomic objects, with an example:
atomic_int guide = ATOMIC_VAR_INIT(42);
It appears to be needed for atomic objects with automatic storage duration, which doesn't apply here.

Since we don't need the automatic storage duration either, this code
should still be correct.

@CLAassistant
Copy link

CLAassistant commented Jul 7, 2017

CLA assistant check
All committers have signed the CLA.

@bkase
Copy link
Contributor Author

bkase commented Jul 7, 2017

@maicki looked at this with me and confirmed this should be okay

Copy link
Contributor

@maicki maicki left a comment

Choose a reason for hiding this comment

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

Looks good to me, but let's have @Adlai-Holler have another look over it.

@Adlai-Holler
Copy link
Member

@bkase We actually do need automatic storage duration on this variable =/.

Since the current implementation of ATOMIC_VAR_INIT simply returns the value unaffected, I'm fine with this patch if it unblocks you, but we should add a comment explaining why we aren't using the API-provided initializer.

@Adlai-Holler
Copy link
Member

Adlai-Holler commented Jul 7, 2017

P.S. A more complete solution to this is to move the implementation of ASDispatchApply into a .m (or .c) file so that it's never compiled by a C++ compiler.

In the header just surround the function declaration by ASDISPLAYNODE_EXTERN_C_BEGIN/END so that C++ can see the function.

@bkase bkase force-pushed the atomic-clang40-workaround branch from 3973c9e to ac6dd31 Compare July 7, 2017 22:04
@bkase
Copy link
Contributor Author

bkase commented Jul 7, 2017

@Adlai-Holler yeah it's blocking the bazel build of master -- how's that comment?

Texture fails to build with this error on clang 4.0:

```
external/Texture/pod_support/Headers/Public/AsyncDisplayKit/ASDispatch.h:32:35: error: illegal initializer type 'atomic_size_t' (aka '_Atomic(size_t)')
  __block atomic_size_t counter = ATOMIC_VAR_INIT(0);
                                  ^
In module 'std' imported from external/Texture/pod_support/Headers/Public/AsyncDisplayKit/ASStackUnpositionedLayout.h:18:
/private/var/tmp/_bazel_bkase/a00d4cbe29902fb63d5778cc19944cd2/external/clang40/bin/../include/c++/v1/atomic:1839:30: note: expanded from macro 'ATOMIC_VAR_INIT'
                             ^
1 error generated.
```

See
http://techqa.info/programming/question/38233019/Initializing-an--atomic-int--with-a-braced-constant--Is-this-valid-C-code--If-so-why-does-it-not-compile-in-clang-

Replacing the intialization with just 0 (and not the macro fixes the
error). For now this is safe because the macro just expands to the
value.
@bkase bkase force-pushed the atomic-clang40-workaround branch from ac6dd31 to e985a57 Compare July 7, 2017 22:06
Copy link
Member

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

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

Down with it

@bkase bkase changed the title Workaround clang4.0 <stdatomic.h> initialization Workaround clang4.0 <stdatomic.h> initialization #trivial Jul 7, 2017
@maicki maicki merged commit 554c688 into TextureGroup:master Jul 7, 2017
jerrymarino pushed a commit to bazel-xcode/PodToBUILD that referenced this pull request Oct 27, 2017
Summary:
This fixes the bazel build (now that we added Weaver) assuming the
following PR gets merged and included in ASDK:
TextureGroup/Texture#426
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
Texture fails to build with this error on clang 4.0:

```
external/Texture/pod_support/Headers/Public/AsyncDisplayKit/ASDispatch.h:32:35: error: illegal initializer type 'atomic_size_t' (aka '_Atomic(size_t)')
  __block atomic_size_t counter = ATOMIC_VAR_INIT(0);
                                  ^
In module 'std' imported from external/Texture/pod_support/Headers/Public/AsyncDisplayKit/ASStackUnpositionedLayout.h:18:
/private/var/tmp/_bazel_bkase/a00d4cbe29902fb63d5778cc19944cd2/external/clang40/bin/../include/c++/v1/atomic:1839:30: note: expanded from macro 'ATOMIC_VAR_INIT'
                             ^
1 error generated.
```

See
http://techqa.info/programming/question/38233019/Initializing-an--atomic-int--with-a-braced-constant--Is-this-valid-C-code--If-so-why-does-it-not-compile-in-clang-

Replacing the intialization with just 0 (and not the macro fixes the
error). For now this is safe because the macro just expands to the
value.
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.

None yet

4 participants