netmail-open / wjelement

advanced, flexible JSON manipulation in C
GNU Lesser General Public License v3.0
108 stars 56 forks source link

#define EXPORT causing macro redefinition warning #108

Open reister-kenneth opened 1 year ago

reister-kenneth commented 1 year ago

In xpl.h EXPORT is being defined. This conflicts with a Microsoft lib

...\windows kits\10\include\10.0.15063.0\um\sqltypes.h

Perhaps a namespace could be added or just a rename of the symbol?

penduin commented 1 year ago

looks like both projects just define it with no value unless you're static-linking, in which case WJE sets it to __declspec(dllexport)

maybe just wrapping our definition in include/xpl.h:

#ifdef _WIN32
# define        WIN_CDECL       __cdecl
# define        WIN_STDCALL     __stdcall
# ifndef COMPILE_AS_STATIC
#  define       EXPORT          __declspec(dllexport)
#  define       IMPORT          __declspec(dllimport)
# else
#  ifndef EXPORT
#   define      EXPORT
#  endif
#  define       IMPORT
#endif

would make your build happy?

it's not the tidiest change (and it leaves unsolved the case of static linking and including sqltypes.h) but i think i'd rather go with that than find-and-replace EXPORT to something else. (internally, we've got enough XPL code and macros for that to be troublesome.)

reister-kenneth commented 1 year ago

When I get some free time I'll apply your changes and see how it likes it.

reister-kenneth commented 1 year ago

Mostly I'm concerned about our upgrade path. For now I've just renamed the symbol to prevent the collision, but the next time we update WJElement it will come back.

reister-kenneth commented 1 year ago

I think you misread the code

#ifndef COMPILE_AS_STATIC

We're compiling the code directly into the project, not as an external DLL/SO so we're not defining the above. I'm getting the

#  define       EXPORT          __declspec(dllexport)

I changed it to

# ifndef COMPILE_AS_STATIC
#ifndef EXPORT
#  define       EXPORT          __declspec(dllexport)
#endif
#  define       IMPORT          __declspec(dllimport)
# else

and all is good.

For a quick fix maybe something more like

# ifndef COMPILE_AS_STATIC
#ifndef XPL_DONT_DEFINE_EXPORT
#  define       EXPORT          __declspec(dllexport)
#endif
#  define       IMPORT          __declspec(dllimport)
# else

And I can just add that to our project -DXPL_DONT_DEFINE_EXPORT

penduin commented 1 year ago

You're right, I had it backwards. I'm glad you found a local fix; we'll mull over what might be the best solution. After looking through our stuff, I think we should indeed be able to change it to XPLEXPORT or something.

It's interesting that you've basically told it to not bother defining EXPORT anymore, and everything still works; I'm not much of a windows guy, so I do not know what __declspec(dllexport) does (or did) for us. Maybe it's obsolete at this point?

minego commented 1 year ago

Have you tried something like:

#include <wjelement.h>
#undef EXPORT
penduin commented 1 year ago

Have you tried something like:

#include <wjelement.h>
#undef EXPORT

I knew I was overthinking this. :^)

reister-kenneth commented 1 year ago

__declspec(dllexport) : https://learn.microsoft.com/en-us/cpp/build/exporting-from-a-dll-using-declspec-dllexport?view=msvc-170 TLDR - It's about name mangling.

I found this snippet in our code

#ifndef EXPORT
#define EXPORT extern "C" __declspec(dllexport)
#endif

With 3 different uses (wjelement, ours, sqltypes.h) I'm starting to think that EXPORT is going to be a "natural" choice for anyone wanting to make that define. That said, my preferred fix at this point would be to rename it. I'm probably going to go rename ours as well shortly.