stephanlachnit commented on code in PR #3600: URL: https://github.com/apache/avro/pull/3600#discussion_r2634154142
########## lang/c++/include/avro/Config.hh: ########## @@ -21,17 +21,29 @@ // Windows DLL support -#ifdef _WIN32 +#ifdef _MSC_VER #pragma warning(disable : 4275 4251) +#endif // _MSC_VER + +#if defined _WIN32 || defined __CYGWIN__ Review Comment: > are introducing three new macros (in addition to the existing AVRO_DECL). Do we intend to use them within Avro library or expect the users of the library to use them? (AVRO_DECL is meant to be used only in headers in the library). I suppose the answer is no for both. You are correct, they are only for use within within avro. > Since macros do not respect namespace or scoping, they pollute the symbol space. Is it better, for example, to directly define AVRO_DECL directly in terms of __declspec or gnu::visibility? I can do this, but I think it hurts readability: it is easier to understand what each macro does (export, import or hide symbols), and then select the corresponding macro for `AVRO_DECL` based on the current compilation setting. > Or, at least, let's comment the macros on their purpose and usage. If they are no objections, I would go with this route instead. -- 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]
