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

compatibility with WIN32_LEAN_AND_MEAN #296

Open
mitjap opened this issue Oct 27, 2023 · 8 comments · May be fixed by #297
Open

compatibility with WIN32_LEAN_AND_MEAN #296

mitjap opened this issue Oct 27, 2023 · 8 comments · May be fixed by #297

Comments

@mitjap
Copy link

mitjap commented Oct 27, 2023

This library is not compatible with WIN32_LEAN_AND_MEAN definition. It fails to find struct timeval which is normally found by windows.h header. But with WIN32_LEAN_AND_MEAN this header is excluded.

We could directly include winsock.h header as it is specified in official documentation (https://learn.microsoft.com/en-us/windows/win32/api/winsock/ns-winsock-timeval).

inline int gettimeofday(struct timeval* tp, void* tzp)

I can prepare a PR for you with this change.

@christian-rauch
Copy link
Collaborator

What's the advantage of using #define WIN32_LEAN_AND_MEAN? According to https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers:

Define WIN32_LEAN_AND_MEAN to exclude APIs such as Cryptography, DDE, RPC, Shell, and Windows Sockets.

So it makes sense that some included headers would then be missing.

@mitjap
Copy link
Author

mitjap commented Oct 27, 2023

Excluding rarely used headers reduces namespace pollution and improves build time. If one want to use it this library should not stand in the way if possible. Instead of including a very general header it should include exactly what it needs.

Here is the link to the fix that we use: modriplanetdoo@45a0a70

@christian-rauch
Copy link
Collaborator

With the diff I better understand what you aim for. I initially understood that you want to use the windows.h header with #define WIN32_LEAN_AND_MEAN and then additionally add the winsock.h header, which would be counterintuitive in my view. Feel free to send a PR with this patch. Also add the required compile flags to the CI in order to make sure that this will not break in future.

Is timeval only directly defined in winsock.h? Is there no specific "time" header that includes this struct?

@mitjap mitjap linked a pull request Oct 28, 2023 that will close this issue
@mitjap
Copy link
Author

mitjap commented Oct 28, 2023

As specified in documentation timeval is defined in winsock.h. It it also recommended to use Winsock2.h which is recommended version of this header.

https://learn.microsoft.com/en-us/windows/win32/api/winsock/ns-winsock-timeval
Header winsock.h (include Winsock2.h)

I am not aware of any other time-related header.

@brmarkus
Copy link

Why is struct timeval (still) used at all might be the question... instead of a plain, standard C or C++ way of dealing with "time"?

@christian-rauch
Copy link
Collaborator

Why is struct timeval (still) used at all might be the question... instead of a plain, standard C or C++ way of dealing with "time"?

Do you know of a standard C way of dealing with time at microsecond resolution?

@brmarkus
Copy link

A quick pointer could be "https://stackoverflow.com/a/67731965" (https://stackoverflow.com/questions/5833094/get-a-timestamp-in-c-in-microseconds/67731965#67731965).

Haven't grepped AprilTag for where timeval is used... why is a timestamp needed to process AprilTags ;-) ?

@christian-rauch
Copy link
Collaborator

A quick pointer could be "https://stackoverflow.com/a/67731965" (https://stackoverflow.com/questions/5833094/get-a-timestamp-in-c-in-microseconds/67731965#67731965).

The response you linked recommends not using timespec_get. So in the end, you are left with the native APIs. The question and the answer are using the POSIX function gettimeofday which is also the one used here.

Haven't grepped AprilTag for where timeval is used... why is a timestamp needed to process AprilTags ;-) ?

The time is used for profiling.

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 a pull request may close this issue.

3 participants