minio / minio-cpp

MinIO C++ Client SDK for Amazon S3 Compatible Cloud Storage
https://minio-cpp.min.io/
Apache License 2.0
135 stars 56 forks source link

Conflict between GetObject() vs GetObject macro in the windows headers (wingdi.h) #134

Closed andy3469 closed 4 months ago

andy3469 commented 7 months ago

Here we continue to find errors with Windows.

The meaning of GetObject is redifined inside the windows header wingdi.h

WINGDIAPI int   WINAPI GetObjectA(__in HANDLE h, __in int c, __out_bcount_opt(c) LPVOID pv);
WINGDIAPI int   WINAPI GetObjectW(__in HANDLE h, __in int c, __out_bcount_opt(c) LPVOID pv);
#ifdef UNICODE
#define GetObject  GetObjectW
#else
#define GetObject  GetObjectA
#endif // !UNICODE

For more context, I'm using the library with Qt on a desktop application so I can't remove this header.

Because of this define, the function GetObject is broken. I didn't check every function for now.

There was the same bug with rapidjson, you can check the issue here: https://github.com/Tencent/rapidjson/issues/1448

andy3469 commented 6 months ago

I've created a fork with a fix. I just wait that the PR #133 is merged as I use it in my version

kobalicek commented 6 months ago

I think the common solutions that other projects use are bad:

#ifdef _MSC_VER
#undef GetObject
#endif

This is bad as this can break user code that relies on GetObject WinAPI function.

#pragma push_macro("GetObject")
#undef GetObject

...

#pragma pop_macro("GetObject")

This is also bad as the user has no way of actually using GetObject as he would get either GetObjectA or GetObjectW if he doesn't explicitly #undef it in his code.

The best way would be temporarily undefining the macro with push_macro/pop_macro in addition to providing inline GetObjectA() and GetObjectW() functions that would just call a real GetObject() - this would work and nobody would notice a thing.

kobalicek commented 6 months ago

I tried here:

https://github.com/minio/minio-cpp/pull/139

I actually don't have a Windows machine here so it's a blind fix - hope it works!

balamurugana commented 6 months ago

@andy3469 Can you confirm the PR https://github.com/minio/minio-cpp/pull/139 works for you?

kobalicek commented 6 months ago

Is there anything that would block merging the PR?

I think only the method applied can be discussed - it has pros and cons, but mostly it works in all cases, no need introducing alternative function or forcing users to undef the macro.

andy3469 commented 5 months ago

Any news for the PR ?

balamurugana commented 5 months ago

@andy3469 I am not able to reproduce the error. Please refer PR https://github.com/minio/minio-cpp/pull/141 and respective discussion is in https://github.com/minio/minio-cpp/pull/139#issuecomment-2111604064

kobalicek commented 5 months ago

It's easy to reproduce the error.

Compile minio-cpp with UNICODE and use it from an app that doesn't define UNICODE or vice versa. Basically now you have to ensure to compile minio-cpp and all code that depends on it having the same UNICODE definition (either defined or not), which is a little bit problematic if you are offering a library for others to consume.

The problem is that if you include <windows.h> the GetObject macro WILL ALWAYS be defined, so in minio-cpp you would actually never have GetObject symbol - it would be either GetObjectA or GetObjectW depending on what <windows.h> defines.

andy3469 commented 5 months ago

@kobalicek I was actually typing the same thing.

When you add #define UNICODE and #include <windows.h> at the top of tests.cc with the main branch I can reproduce the error.