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

Fixed a GetObject() function colliding with WinAPI macro #139

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

kobalicek
Copy link
Contributor

@kobalicek kobalicek commented Apr 18, 2024

Fixes #134

@kobalicek
Copy link
Contributor Author

@balamurugana Ping :)

@balamurugana
Copy link
Member

I am trying to reproduce the error with Github CI by PR #141. If I include windows.h, it works without this PR and including wingdi.h fails with other errors.

@kobalicek
Copy link
Contributor Author

The failure doesn't seem to be caused by minio-cpp at all. I'm not a windows expert, but I think that since <wingdi.h> doesn't include any other headers it cannot be just included without including others first - most likely at least <windef.h>. WinAPI is old and usually people just include a super header <windows.h> to get most of the core stuff.

@harshavardhana
Copy link
Member

PTAL @balamurugana

@balamurugana
Copy link
Member

@harshavardhana We need a working reproducer of the issue. Please refer #141

@kobalicek
Copy link
Contributor Author

That's a bad reproducer - create a fresh project and include <wingdi.h> and you will see the same failure.

The change is actually solid - it creates the right symbol (without A or W suffix) and then offers inlines in case <windows.h> was included. But it's transparent for users.

@balamurugana
Copy link
Member

That's a bad reproducer - create a fresh project and include <wingdi.h> and you will see the same failure.

The change is actually solid - it creates the right symbol (without A or W suffix) and then offers inlines in case <windows.h> was included. But it's transparent for users.

@kobalicek Could you add below test case for this PR?

diff --git a/tests/tests.cc b/tests/tests.cc
index e06e1b3..866de1c 100644
--- a/tests/tests.cc
+++ b/tests/tests.cc
@@ -15,6 +15,11 @@
 //
 // SPDX-License-Identifier: Apache-2.0
 
+#ifdef _WIN32
+#define UNICODE
+#include <windows.h>  // To Test https://github.com/minio/minio-cpp/issues/134
+#endif
+
 #include <miniocpp/args.h>
 #include <miniocpp/client.h>
 #include <miniocpp/http.h>

@harshavardhana
Copy link
Member

That's a bad reproducer - create a fresh project and include <wingdi.h> and you will see the same failure.
The change is actually solid - it creates the right symbol (without A or W suffix) and then offers inlines in case <windows.h> was included. But it's transparent for users.

@kobalicek Could you add below test case for this PR?

diff --git a/tests/tests.cc b/tests/tests.cc
index e06e1b3..866de1c 100644
--- a/tests/tests.cc
+++ b/tests/tests.cc
@@ -15,6 +15,11 @@
 //
 // SPDX-License-Identifier: Apache-2.0
 
+#ifdef _WIN32
+#define UNICODE
+#include <windows.h>  // To Test https://github.com/minio/minio-cpp/issues/134
+#endif
+
 #include <miniocpp/args.h>
 #include <miniocpp/client.h>
 #include <miniocpp/http.h>

is this added ? PTAL @kobalicek

@harshavardhana
Copy link
Member

@kobalicek ^^ PTAL in-case you missed.

@kobalicek kobalicek merged commit cd4ef14 into main Jul 3, 2024
4 checks passed
@Wiladams
Copy link

Wiladams commented Jul 3, 2024

Just for posterity, this is how I include "windows" in my app

#include <SDKDDKVer.h>

#define WIN32_LEAN_AND_MEAN
#define NOMINMAX
#define _WINSOCK_DEPRECATED_NO_WARNINGS 1

#include <windows.h>

@harshavardhana harshavardhana deleted the get_object_fix branch July 3, 2024 15:13
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.

Conflict between GetObject() vs GetObject macro in the windows headers (wingdi.h)
4 participants