lidavidm commented on code in PR #14193: URL: https://github.com/apache/arrow/pull/14193#discussion_r977602464
########## cpp/src/arrow/util/visibility.h: ########## @@ -18,28 +18,66 @@ #pragma once #if defined(_WIN32) || defined(__CYGWIN__) +// Windows + #if defined(_MSC_VER) #pragma warning(disable : 4251) #else #pragma GCC diagnostic ignored "-Wattributes" #endif +#if defined(__cplusplus) && (defined(__GNUC__) || defined(__clang__)) +// Use C++ attribute syntax where possible to avoid GCC parser bug +// (https://stackoverflow.com/questions/57993818/gcc-how-to-combine-attribute-dllexport-and-nodiscard-in-a-struct-de) +#define ARROW_DLLEXPORT [[gnu::dllexport]] // NOLINT(whitespace/braces) +#define ARROW_DLLIMPORT [[gnu::dllimport]] // NOLINT(whitespace/braces) +#else +#define ARROW_DLLEXPORT __declspec(dllexport) +#define ARROW_DLLIMPORT __declspec(dllimport) +#endif + #ifdef ARROW_STATIC #define ARROW_EXPORT +#define ARROW_FRIEND_EXPORT +#define ARROW_TEMPLATE_EXPORT #elif defined(ARROW_EXPORTING) -#define ARROW_EXPORT __declspec(dllexport) +#define ARROW_EXPORT ARROW_DLLEXPORT +// For some reason [[gnu::dllexport]] doesn't work well with friend declarations +#define ARROW_FRIEND_EXPORT __declspec(dllexport) +#define ARROW_TEMPLATE_EXPORT ARROW_DLLEXPORT #else -#define ARROW_EXPORT __declspec(dllimport) +#define ARROW_EXPORT ARROW_DLLIMPORT +#define ARROW_FRIEND_EXPORT __declspec(dllimport) +#define ARROW_TEMPLATE_EXPORT ARROW_DLLIMPORT #endif #define ARROW_NO_EXPORT #define ARROW_FORCE_INLINE __forceinline -#else // Not Windows + +#else + +// Non-Windows + +#define ARROW_FORCE_INLINE + +#if defined(__cplusplus) && (defined(__GNUC__) || defined(__clang__)) #ifndef ARROW_EXPORT -#define ARROW_EXPORT __attribute__((visibility("default"))) +#define ARROW_EXPORT [[gnu::visibility("default")]] // NOLINT(whitespace/braces) #endif #ifndef ARROW_NO_EXPORT -#define ARROW_NO_EXPORT __attribute__((visibility("hidden"))) -#define ARROW_FORCE_INLINE +#define ARROW_NO_EXPORT [[gnu::visibility("default")]] // NOLINT(whitespace/braces) Review Comment: Should this be "hidden"? ########## cpp/src/arrow/util/future.h: ########## @@ -315,7 +315,7 @@ class ARROW_EXPORT FutureImpl : public std::enable_shared_from_this<FutureImpl> /// The consumer API allows querying a Future's current state, wait for it /// to complete, and composing futures with callbacks. template <typename T> -class ARROW_MUST_USE_TYPE Future { +class [[nodiscard]] Future { // NOLINT(whitespace/braces) Review Comment: Maybe we should update this? https://github.com/apache/arrow/blob/b0d74bd0ee964d30d715fe6123c87a901bad369e/cpp/build-support/cpplint.py#L3428-L3432 Upstream has updated: https://github.com/cpplint/cpplint/blob/fa12a0bbdafa15291276ddd2a2dcd2ac7a2ce4cb/cpplint.py#L3799-L3804 ########## cpp/src/arrow/util/future.h: ########## @@ -315,7 +315,7 @@ class ARROW_EXPORT FutureImpl : public std::enable_shared_from_this<FutureImpl> /// The consumer API allows querying a Future's current state, wait for it /// to complete, and composing futures with callbacks. template <typename T> -class ARROW_MUST_USE_TYPE Future { +class [[nodiscard]] Future { // NOLINT(whitespace/braces) Review Comment: Maybe we should just update the cpplint version first -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
