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

Do not return pointer to local from get_timezone #51

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

skeeto
Copy link
Contributor

@skeeto skeeto commented Jan 17, 2025

The pointed at variable was also a wchar_t array, not char, and so also convert the contents to UTF-8. The static buffer is sized for the worst case and the conversion will always succeed.


I went with a static buffer because it fits the semantics of the unix version, and a similar approach is used in normalize_emoji.

@da-luce
Copy link
Owner

da-luce commented Jan 17, 2025

image

It would be nice if the time zone name wasn't so long. Does Windows provide a native way to obtain the abbreviated name?

@skeeto
Copy link
Contributor Author

skeeto commented Jan 17, 2025 via email

@da-luce
Copy link
Owner

da-luce commented Jan 17, 2025

I like the idea of using the bias. To fit within the display the UTC could be dropped, so it'd just be "-05:00," much like the git log output as you mentioned.

@da-luce da-luce self-requested a review January 17, 2025 17:55
@da-luce da-luce linked an issue Jan 17, 2025 that may be closed by this pull request
src/main.c Outdated Show resolved Hide resolved
The pointed at variable was also a wchar_t array, not char. Since Win32
does not provide abbreviated timezone names, express the timezone as a
an RFC3339-style numeric offset.
@skeeto skeeto force-pushed the fix-pointer-to-local-return branch from 126032b to e366967 Compare January 17, 2025 23:35
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

see 2 files with indirect coverage changes

@da-luce da-luce merged commit aa9587b into da-luce:dev Jan 17, 2025
15 checks passed
@skeeto skeeto deleted the fix-pointer-to-local-return branch January 18, 2025 00:42
@skeeto
Copy link
Contributor Author

skeeto commented Jan 18, 2025

Thanks! Your program is quite fascinating and interesting. I've been poking around the code since you shared it on reddit, hence these two PRs.

@da-luce
Copy link
Owner

da-luce commented Jan 18, 2025

Thank you! I saw your fork and am really impressed with how simple you were able to get the Makefile. The Meson build is currently far more verbose than I'd like, but there are just so many weird cross-platform edge cases to handle. I'd consider moving to a Makefile like yours but that would require Windows users to have a GNU environment available (I'd also hate to give up Meson's arguably nicer syntax). CMake was originally a possibility but have only heard horror stories.

Thanks again for your PRs, and any future ideas or contributions are always welcome!

@skeeto
Copy link
Contributor Author

skeeto commented Jan 18, 2025 via email

@da-luce
Copy link
Owner

da-luce commented Jan 18, 2025

I checked out w64devkit and it's really neat! It's a bit odd Unicode isn't working for you. The precompiled binary for Windows displays Unicode correctly when I've tested on my desktop. ncurses/PDCurses seem to be super environment dependent when it comes to character support (I can't get certain Unicode characters to display on my Mac for the life of me), so it could be any number of things--but I'm not sure PDCurses is to blame in this instance.

@rmyorston
Copy link

I saw your fork and am really impressed with how simple you were able to get the Makefile.

Not only simple, also POSIX 2024 compliant!

@skeeto
Copy link
Contributor Author

skeeto commented Jan 18, 2025

POSIX 2024 compliant

I was going to object because I used !=, but your excellent summary cleared that up!

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.

Broken time zone on Windows
3 participants