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 clang 14 __has_declspec_attribute restriction #1723

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

liam-x
Copy link

@liam-x liam-x commented Oct 7, 2022

fix #1710

clang 14 impose a new restriction on __has_declspec_attribute, the argument can not be empty.
llvm/llvm-project#53269

cppresetsdk cpprest_compat.h translate dllimport to empty which cause compilation failure on clang 14.

A workaround by removing define dllimport and add no-unknown-attributes flag to suppress the compilation warning from clang on macOS and Linux.

@liam-x liam-x marked this pull request as ready for review October 14, 2022 04:42
@@ -29,7 +29,9 @@
#else // ^^^ _WIN32 ^^^ // vvv !_WIN32 vvv

#define __declspec(x) __attribute__((x))
#define dllimport
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a good way to fix this. There are not that many appearances of __declspec in the codebase, and most are already behind visibility macros, the remaining are noinline, noreturn, and novtable. Noreturn can probably be replaced by writing [[noreturn]] directly, noinline should be replaced by something like

#ifdef _MSC_VER
#define CPPREST_NOINLINE __declspec(noinline)
#else
#define CPPREST_NOINLINE __attribute__((noinline))
#endif

and then replacing __declspec(noinline) usages with CPPREST_NOINLINE

the usages with dllimport/dllexport should be replaced by a proper set of visibility macros, cmake can generate them but they are also easy to write, something like:

#ifdef _MSC_VER
    #define CPPREST_API_CLASS
    #if defined(CPPREST_BUILDING) && defined(CPPREST_DLL)
        #define CPPREST_API __declspec(dllexport)
    #elif defined(CPPREST_DLL)
        #define CPPREST_API __declspec(dllimport)
    #else
        #define CPPREST_API
    #endif
#else
    // it might be a good idea to do something special for static libraries here,
    // since this could result in apps exporting symbols to loaded DSOs instead of those
    // DSOs loading cpprestsdk themselves.
    #define CPPREST_API_CLASS __attribute__((visibility("default")))
    #define CPPREST_API __attribute__((visibility("default")))
    #endif
#endif

any classes used as exceptions (or generally if the RTTI information needs exporting) nned to be annotated as CPPREST_API_CLASS, this should be a seperate macro because on windows applying dllexport to a class can export all it's bases, which is extremely annoying

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.

cpprest_compat breaks cURL header when building with Clang 14
2 participants