Fix use of undefined pointer types in definitions

computerquip commented

2018-11-20 15:51:36 -06:00

(Migrated from github.com)

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.

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 commented

2018-11-20 15:51:47 -06:00

(Migrated from github.com)

All CLA requirements met.

[![CLA assistant check](https://cla.opensource.microsoft.com/pull/badge/signed)](https://cla.opensource.microsoft.com/mixer/ftl-sdk?pullRequest=78) <br/>All CLA requirements met.

SuslikV commented

2019-02-01 03:50:25 -06:00

(Migrated from github.com)

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.

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 commented

2019-02-02 05:50:57 -06:00

(Migrated from github.com)

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

github.com/mixer/ftl-sdk@d5340c5842

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.

Is there a push to remove Winsock2.h, who understands its meaning? https://github.com/mixer/ftl-sdk/commit/d5340c5842cb7ac551bcf0e5c65375cf49f933e9 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 commented

2019-02-06 01:09:53 -06:00

(Migrated from github.com)

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.

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 commented

2019-02-09 22:42:22 -06:00

(Migrated from github.com)

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.

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 commented

2019-04-24 00:45:58 -05:00

(Migrated from github.com)

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.

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.

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'computerquip/clang-cl-support:computerquip/clang-cl-support'

git switch 'computerquip/clang-cl-support'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'master'

git merge --no-ff 'computerquip/clang-cl-support'

git switch 'computerquip/clang-cl-support'

git rebase 'master'

git switch 'master'

git merge --ff-only 'computerquip/clang-cl-support'

git switch 'computerquip/clang-cl-support'

git rebase 'master'

git switch 'master'

git merge --no-ff 'computerquip/clang-cl-support'

git switch 'master'

git merge --squash 'computerquip/clang-cl-support'

git switch 'master'

git merge --ff-only 'computerquip/clang-cl-support'

git switch 'master'

git merge 'computerquip/clang-cl-support'

git push origin 'master'

Rows
Columns

Fix use of undefined pointer types in definitions #78

Checkout

Merge