Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Fix use of undefined pointer types in definitions #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

computerquip
Copy link

This coincidentally also adds support for clang-cl. There's two problems being solved here.

The types timeeval and timezone are not defined where they're used in the gettimeofday.h header
timezone isn't defined by the WinAPI headers anywhere. timeeval is only defined in Winsock2.h.

The implementation of gettimeofday() here doesn't actually use the timezone structure so it may be defined as an opaque pointer without definition. timeeval is used however so it must have a definition, so we must include Winsock2.h in the header. Since the definition in the header didn't originally have a type associated with it, it could be interpreted as a different type from that of the one provided by WinSock2.h. clang-cl believes it to be an anonymous structure in the header so it thinks the function type is different between header declaration and implementation.

@msftclas
Copy link

msftclas commented Nov 20, 2018

CLA assistant check
All CLA requirements met.

@SuslikV
Copy link

SuslikV commented Feb 1, 2019

Whole header because of 1 small structure?!
https://docs.microsoft.com/en-us/windows/desktop/api/winsock2/ns-winsock2-timeval
Also, winsock inclusion may depend on WIN32_LEAN_AND_MEAN usage.

Possible implementation:

#ifndef _WINSOCKAPI_
struct timeval {
  long tv_sec; /* Time interval, in seconds */
  long tv_usec; /* Time interval, in microseconds (if needed). The time interval is a combination of the values in tv_sec and tv_usec members */
};
#endif

just for info, unused structure is also small:

struct timezone {
  int  tz_minuteswest; /* minutes W of Greenwich */
  int  tz_dsttime;     /* type of dst correction */
};

I think, if function is replacement - it should replace everything.

@tyiki-badwell
Copy link
Contributor

Is there a push to remove Winsock2.h, who understands its meaning?

d5340c5

There may be missing instructions to write to CMakeLists.txt.

However, care to timezone is necessary.
It might be better to replace it with clock_gettime() and alias if possible.

@computerquip
Copy link
Author

computerquip commented Feb 6, 2019

We could just make our own function that returns the time specification we want. For instance, obs uses os_gettime_ns which wraps over platform specifics. Here's an example of a similar function in action:

      gettimeofday(&frameTime, NULL); // NOTE! In a real app these timestamps should come from the samples!
      gettimeofday(&proc_end_tv, NULL);
      timeval_subtract(&proc_delta_tv, &proc_end_tv, &proc_start_tv);

      video_send_delay += video_time_step;
      time_delta = (float)timeval_to_ms(&proc_delta_tv);
      video_send_delay -= time_delta;

      if (video_send_delay > 0)
      {
        gettimeofday(&profile_start, NULL);
        sleep_ms((int)video_send_delay);
        gettimeofday(&profile_stop, NULL);
        timeval_subtract(&profile_delta, &profile_stop, &profile_start);
        actual_sleep = (float)timeval_to_ms(&profile_delta);

to

      uint64_t frame_time = os_gettime_us();
      uint64_t end_time = os_gettime_us();

      uint64_t delta_time = end_time - start_time;

      video_send_delay += video_time_step;
      delta_time_ms = delta_time * 0.001f; /* Convert from micro to millisecond */
      video_send_delay -= delta_time_ms;

      if (video_send_delay > 0)
      {
        uint64_t profile_start = os_gettime_us();
        sleep_ms((int)video_send_delay);
        uint64_t profile_stop = os_gettime_us();
        uint64_t profile_delta = profile_stop - profile_start;
        actual_sleep = profile_delta * 0.001f;

I'm not sure the wanted precision of the clock so I guess that could be nanoseconds above and then just convert on the fly to whatever measurement wanted, perhaps make some convenience functions to do so. I'm also not sure how involved that is to change so maybe take the suggestion with a grain of salt.

@tyiki-badwell
Copy link
Contributor

tyiki-badwell commented Feb 10, 2019

Wait as I will do 2 points.

1.To check the bad influence when adding Winsock 2.h to the header file.

2.To find the resolution of the time that is really needed.

@kkartaltepe
Copy link

I am also looking forward to the removal of this dependence on MSVC semantics, and likely standards non-compliant code, but defining a struct in a function declaration is quite the edge case.

Personally, these changes simply make the code compile on clang-cl, so there is little gain in delaying it to resolve the pre-existing bugs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants