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

timediff.[ch]: add curlx helper functions for timeval conversions #8595

Closed
wants to merge 1 commit into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Mar 15, 2022

Also move timediff_t definitions from timeval.h to timediff.h and
then make timeval.h include the new standalone-capable timediff.h.


New version of #5888.

@mback2k mback2k self-assigned this Mar 15, 2022
lib/Makefile.inc Outdated Show resolved Hide resolved
@jay
Copy link
Member

@jay jay commented Mar 15, 2022

Should be added to generate.bat curlx section:

diff --git a/projects/generate.bat b/projects/generate.bat
index 0bbe3fb..efded08 100644
--- a/projects/generate.bat
+++ b/projects/generate.bat
@@ -215,6 +215,7 @@ rem
       call :element %1 lib "curl_multibyte.c" %3
       call :element %1 lib "version_win32.c" %3
       call :element %1 lib "dynbuf.c" %3
+      call :element %1 lib "timeutil.c" %3
     ) else if "!var!" == "CURL_SRC_X_H_FILES" (
       call :element %1 lib "config-win32.h" %3
       call :element %1 lib "curl_setup.h" %3
@@ -225,6 +226,7 @@ rem
       call :element %1 lib "curl_multibyte.h" %3
       call :element %1 lib "version_win32.h" %3
       call :element %1 lib "dynbuf.h" %3
+      call :element %1 lib "timeutil.h" %3
     ) else if "!var!" == "CURL_LIB_C_FILES" (
       for /f "delims=" %%c in ('dir /b ..\lib\*.c') do call :element %1 lib "%%c" %3
     ) else if "!var!" == "CURL_LIB_H_FILES" (

and also CURLX_ sections in src/Makefile.inc and tests/server/Makefile.inc

@mback2k mback2k force-pushed the timeval-helper-functions branch 7 times, most recently from 7d9fefd to c3f6820 Compare Mar 15, 2022
@bagder
Copy link
Member

@bagder bagder commented Mar 15, 2022

Maybe one day we should name all these curlx_* header/source files in special way, or put them in a subdir or something, to make it more apparent that they are magic and are used outside of the regular source dir.

@mback2k mback2k force-pushed the timeval-helper-functions branch 2 times, most recently from 8276bb1 to 1cceafa Compare Mar 21, 2022
@mback2k
Copy link
Member Author

@mback2k mback2k commented Mar 21, 2022

Before we think of additional changes, do you think this PR is ready from a code/content change perspective? @bagder @jay

lib/vtls/gskit.c Outdated Show resolved Hide resolved
@jay
Copy link
Member

@jay jay commented Mar 22, 2022

Any curlx refactor would be separate from this. Aside from the gskit comment I just left this pr looks fine to me.

lib/vtls/gskit.c Outdated Show resolved Hide resolved
@mback2k
Copy link
Member Author

@mback2k mback2k commented Apr 6, 2022

@jay what do you think of the solution in the most recent version of this PRs changes to gskit.c now?

@jay
Copy link
Member

@jay jay commented Apr 7, 2022

@jay what do you think of the solution in the most recent version of this PRs changes to gskit.c now?

LGTM. I've marked the review conversations as resolved.

Also move timediff_t definitions from timeval.h to timediff.h and
then make timeval.h include the new standalone-capable timediff.h.

Reviewed-by: Jay Satiro

Supersedes curl#5888
Closes curl#8595
@mback2k mback2k force-pushed the timeval-helper-functions branch from e27e1af to ff20b26 Compare Apr 7, 2022
@mback2k mback2k changed the title timeutil.[ch]: add curlx helper functions for timeval conversions timediff.[ch]: add curlx helper functions for timeval conversions Apr 7, 2022
@mback2k mback2k marked this pull request as ready for review Apr 7, 2022
@mback2k mback2k requested a review from bagder Apr 7, 2022
@mback2k
Copy link
Member Author

@mback2k mback2k commented Apr 8, 2022

CI is , can this go in during feature freeze @bagder?

bagder
bagder approved these changes Apr 17, 2022
Copy link
Member

@bagder bagder left a comment

👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants