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

mingw: use modern strftime implementation if possible #2574

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

rimrul
Copy link
Member

@rimrul rimrul commented Apr 5, 2020

Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

@rimrul
Copy link
Member Author

rimrul commented Apr 5, 2020

Looking back at the diff, it might make sense to move _fallback_strftime and it's initialization into mingw_strftime().

@dscho
Copy link
Member

dscho commented Apr 5, 2020

In addition to moving it inside, can we also initialize it directly, i.e. static int (*fallback)(...) = strftime;?

@rimrul
Copy link
Member Author

rimrul commented Apr 5, 2020

static int

Microsoft says it should be size_t, not int
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strftime-wcsftime-strftime-l-wcsftime-l?view=vs-2019

(*fallback)(...) = strftime;

yeah, I guess that works. I'll prepare an updated version.

@dscho
Copy link
Member

dscho commented Apr 5, 2020

Sorry, on my phone, couldn't cut 'n paste.

Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes git-for-windows#2495

Signed-off-by: Matthias Aßhauer <mha1993@live.de>
@dscho dscho merged commit 8644094 into git-for-windows:master Apr 7, 2020
@dscho
Copy link
Member

dscho commented Apr 7, 2020

Perfect, @rimrul you're awesome!

@dscho dscho added this to the Next release milestone Apr 7, 2020
dscho added a commit to git-for-windows/build-extra that referenced this pull request Apr 7, 2020
Git [now accepts more date
formats](git-for-windows/git#2574) such as
`%g` and `%V`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit that referenced this pull request Apr 7, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request Apr 7, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request Apr 7, 2020
mingw: use modern strftime implementation if possible
dscho added a commit that referenced this pull request Apr 8, 2020
mingw: use modern strftime implementation if possible
dscho added a commit that referenced this pull request Apr 8, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request Apr 8, 2020
mingw: use modern strftime implementation if possible
dscho added a commit that referenced this pull request Apr 8, 2020
mingw: use modern strftime implementation if possible
dscho added a commit that referenced this pull request Apr 8, 2020
mingw: use modern strftime implementation if possible
dscho added a commit that referenced this pull request Apr 9, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request Apr 14, 2020
mingw: use modern strftime implementation if possible
derrickstolee pushed a commit to microsoft/git that referenced this pull request Apr 14, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request Apr 14, 2020
mingw: use modern strftime implementation if possible
@TheStormN
Copy link

@dscho @rimrul
Sorry to bring this back, I saw it from the new version changelog, The current code doesn't seem very optimal. Why try to load and resolve the address of strftime every time? Just do it once per startup and use the pointer.

This might not seem like a big deal for programs that use strftime rarely but because it is inside the mingw implementation I think it needs to be more optimal.

@rimrul
Copy link
Member Author

rimrul commented Apr 15, 2020

That's exactly what we do. INIT_PROC_ADDR caches the address.

See it's implementation here: https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/compat/win32/lazyload.h#L31-L55

@TheStormN
Copy link

Oh, ok, thank you for the clarification and sorry for the misunderstanding. :)

dscho added a commit that referenced this pull request Apr 17, 2020
mingw: use modern strftime implementation if possible
dscho added a commit that referenced this pull request Apr 18, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request Apr 20, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request Apr 20, 2020
mingw: use modern strftime implementation if possible
derrickstolee pushed a commit to microsoft/git that referenced this pull request Apr 20, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request Apr 24, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request May 1, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request May 9, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request May 15, 2020
mingw: use modern strftime implementation if possible
git-for-windows-ci pushed a commit that referenced this pull request May 29, 2020
mingw: use modern strftime implementation if possible
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.

3 participants