higan-emu / libco

libco is a cooperative multithreading library written in C89.
Other
125 stars 25 forks source link

Proper cross-platform settings.h #7

Closed namandixit closed 4 years ago

namandixit commented 4 years ago

The thread_local and alignas macros in settings.h were defined weird. I have fixed them for the most common cases, and have provided a overload mechanism as a backup.

@Screwtapello @MerryMage Do you want PRs for libco here or in the higan repo? I am not very familiar with git tree workflow.

merryhime commented 4 years ago

PRs here are what's intended afaik.

Screwtapello commented 4 years ago

I'm not a super-expert on this stuff, but it looks reasonable to me.

For alignas(), I assume you check for MSC_VER before checking __STDC_VERSION__ because we just assume MSVC will never support modern versions of C, only C++?

namandixit commented 4 years ago

For alignas(), I assume you check for MSC_VER before checking __STDC_VERSION__ because we just assume MSVC will never support modern versions of C, only C++?

Even worse, since MSVC supports _Generic now (in beta I think), they might decide to set __STDC_VERSION__ to 2011, specifications be damned. After 20 years of C99 non-support, it's just good sense to not rely on Microsoft's C compliance anymore.

namandixit commented 4 years ago

Now that I think of it, I should also move thread_local out too. Don't merge yet, I'll clean the whole thing up a bit more and document the intentions.

namandixit commented 4 years ago

@Screwtapello Should be good now, is this clearer?

namandixit commented 4 years ago

I have remove the double comment and have squashed all the changes in a proper commit, hopefully nothing went wrong

merryhime commented 4 years ago

As I don't really I enjoy deeply nested preprocessor directives, I feel this could be written more succinctly as:

#if !defined(thread_local) // User can override thread_local for obscure compilers
  #if !defined(LIBCO_MP) // Running in single-threaded environment
    #define thread_local
  #elif defined(_MSC_VER)
    #define thread_local __declspec(thread)
  #elif defined(__cplusplus) && __cplusplus >= 201103L
    // thread_local is a C++11 feature
  #elif defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
    #include <threads.h>
  #elif defined(__clang__) || defined(__GNUC__)
    #define thread_local __thread
  #endif
#endif

/* In alignas(a), 'a' should be a power of two that is at least the type's
   alignment and at most the implementation's alignment limit.  This limit is
   2**13 on MSVC. To be portable to MSVC through at least version 10.0,
   'a' should be an integer constant, as MSVC does not support expressions
   such as 1 << 3.
   The following C11 requirements are NOT supported on MSVC:
   - If 'a' is zero, alignas has no effect.
   - alignas can be used multiple times; the strictest one wins.
   - alignas (TYPE) is equivalent to alignas (alignof (TYPE)).
*/
#if !defined(alignas)
  #if defined(_MSC_VER)
    #define alignas(bytes) __declspec(align(bytes))
  #elif defined(__cplusplus) && __cplusplus >= 201103L
    // alignas is a C++11 feature
  #elif defined(__STDC_VERSION__) && __STDC_VERSION__  >= 201112L
    #include <stdalign.h>
  #elif defined(__clang__) || defined(__GNUC__)
    #define alignas(bytes) __attribute__ ((aligned (bytes)))
  #else
    #define alignas(bytes)
  #endif
#endif

Before you ask, yes, && shortcircuits in a preprocessor if statement, so the fact the symbols are undefined doesn't matter.

namandixit commented 4 years ago

@MerryMage That's similar to how I had written it initially, but it was apparently a bit confusing; so, I decided to spell everything out and squash all the previous commits. The reason I decided to separate out C and C++ was to allow MSVC to catch up at some point with C11 compliance.

Also, in your implementation, a fallback empty assignment to thread_local is missing if all other conditions fail (if LIBCO_MP is defined on non-GCC/Clang/MSVC compilers with C90/99 or C++98/03).

namandixit commented 4 years ago

@MerryMage Do you wish me to replace the code in this PR with the one suggested by you? Is there anything else blocking this PR?

merryhime commented 4 years ago

No, @Screwtapello feel free to merge this if you think it's appropriate.

Screwtapello commented 4 years ago

Sorry about the delay, @namandixit, many thanks for contributing!