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

Move repetitive content to detours.h under #ifdef DETOURS_INTERNAL. #6

Merged
merged 1 commit into from May 15, 2019

Conversation

jaykrell
Copy link
Member

No description provided.

@jaykrell jaykrell changed the title Move repetitive content to detours.h undef #ifdef DETOURS_INTERNAL. Move repetitive content to detours.h under #ifdef DETOURS_INTERNAL. May 12, 2018
@dtarditi
Copy link
Contributor

This change looks fine, other than the DETOURS_INTERNAL #ifdef being at the top of Detours.h. It could confuse people reading the header file to see this as the first thing. Could you move the code lower in Detours.h, where other DETOURS_INTERNAL code is located.

@jaykrell
Copy link
Member Author

Just move into the existing slightly later DETOURS_INTERNAL block? Yes that looks like it will work.

@dtarditi
Copy link
Contributor

Yes, just moving it slightly later would be great.

@jaykrell
Copy link
Member Author

jaykrell commented Jan 25, 2019

If you move it down just one "section" you get:

z:\s\detours\src\detours.h(47) : fatal error C1189: #error : Unknown architecture (x86, amd64, ia64, arm, arm64)
modules.cpp

because _X86_ etc. are defined by windows.h.

I probably put this right at the top, because everyone else was doing it before including detours.h.

@jaykrell
Copy link
Member Author

Prefer repetition and prettier detours.h? Or just delayed?

@dtarditi
Copy link
Contributor

Could we leave the #include "windows.h" in the .c files, and the move the code down in Detours.h? Or does the #define _ARM_WINAPI_PARTITION_DESKTOP_SDK_AVAILABLE need to precede the include of "windows.h"?

@jaykrell
Copy link
Member Author

jaykrell commented Mar 2, 2019

I think what you hint at is true -- _ARM_WINAPI_PARTITION_DESKTOP_SDK_AVAILABLE must be early. At least in the past. ARM development was by default limited to "the store", unless explicitly enable "desktop" development.
Oh, um, another option is /D_ARM_WINAPI_PARTITION_DESKTOP_SDK_AVAILABLE in Makefile. I like source to be more build-system-independent though.

Copy link
Contributor

@dtarditi dtarditi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for looking at the alternatives.

@dtarditi dtarditi merged commit 404c153 into microsoft:master May 15, 2019
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.

None yet

2 participants