Refactor time parsing with thread-safe localtime and leap year fix

[KinshukSS2]

replace non-thread-safe localtime() with localtime_r() (POSIX) / localtime_s() (Windows) in hgridshift and vgridshift +t_final=now handling.

also fix leap year calculation: use 366 days for leap years instead of hardcoded 365.

Closes #4691

[rouault]

@kbevers I see the original formula to translate to decimal year comes from 0b5c362 . I remember that ISO 19111 doesn't mention anything about that (or maybe that leap years don't matter). Anyway in src/conversions/unitconvert.cpp, we do take into leap years. Should we align hgridshift and vgridship on that ?

[jjimenezshaw]

Maybe the refactoring can be done now

[kbevers]

@kbevers I see the original formula to translate to decimal year comes from 0b5c362 . I remember that ISO 19111 doesn't mention anything about that (or maybe that leap years don't matter). Anyway in src/conversions/unitconvert.cpp, we do take into leap years. Should we align hgridshift and vgridship on that ?

It's been a while so I don't remember my thought process but I guess I didn't see any reason to complicate things. Usually we are looking a rates of mm or cm pr year in a deformation model. The difference between 1/365 and 1/366 of a year is neglicable and well within the uncertainty of any imperical model. But accounting for leap years is more correct and with a bit of refactoring everything will be a little bit nicer for the next person.

[KinshukSS2]

I have refactored the duplicated t_final code from hgridshift.cpp and vgridshift.cpp into a shared function pj_parse_t_final()
did some thorough testing of builds as well as functionalities to make sure it works as intended
pls do give a look into it and happy to make changes if it suits the intended plan of making a reusable func
note: i am seeing a failure in the file_api test ,hoping it is not related to the refactoring

[kbevers]

The refactored time function looks good. I'd like to see a more descriptive commit message for the clang-format fix, as it's not clear to me what is actually being fixed.

[KinshukSS2]

@kbevers

i think it would be better if i will squash all the 3 commits into one in order to not create any chaos and give a proper description

[kbevers]

i think it would be better if i will squash all the 3 commits into one in order to not create any chaos and give a proper description

That's an option, yes. It still doesn't explain what's going on with the changes to satisfy clang-format. If I look at this commit in ten years I'd like to know why changes regarding opening of grid files is mixed with refactoring of functionality to parse time. Ideally this PR is rebased into two commits: one with the changes to time parsing and the other that fixes whatever clang-format complains about.

[KinshukSS2]

@kbevers so you would recommend me splitting it into two commits one stating about the clang fixtures (due to CI failures i faced) and the other one abt refactoring ?
wanted to confirm before i proceed

[kbevers]

Yes

[KinshukSS2]

@kbevers the clang format issue from the ci failure was specifically abt formating the new code (one i added)

it would be counterproductive to create one commit with improperly formatted new code and then a second commit just to fix that formatting

the refactoring should contain the properly formatted code from the start istelf
hoping i've understood the way you interpreted the situation

[kbevers]

@KinshukSS2 in a previous rendition of this PR you had changes in src/transformations/gridshift.cpp (seen in this commit 1408e44), those are what I was asking for more information on. They aren't present in the latest (and only) commit, so no need to dwelve into that.

[jjimenezshaw]

@KinshukSS2 in a previous rendition of this PR you had changes in src/transformations/gridshift.cpp (seen in this commit 1408e44), those are what I was asking for more information on. They aren't present in the latest (and only) commit, so no need to dwelve into that.

Those changes are about using std::lock_guard<std::mutex> instead of lock and unlock. It is not about clang-format.

[kbevers]

Those changes are about using std::lock_guardstd::mutex instead of lock and unlock. It is not about clang-format.

Well, they were initially put in a commit that said "fix clang-format" which has since been force-pushed away. And now the changes aren't here at all. The thing I've been going on about was to get an explanation for what was going on, as it didn't look like anything to do with the stated purpose of the PR. But that discussion is futile now that those lines of code are gone from the PR.

This is ready to merge once the tests have run successfully.