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

replace _DATE_FMT with format specifiers #2596

Closed
wants to merge 1 commit into from

Conversation

salazaj
Copy link
Contributor

@salazaj salazaj commented Aug 13, 2014

Bring into conformance with Open Group Base Specifications. Or bypass nl_langinfo to pass the format specifiers directly and conform to ISO.

@salazaj salazaj changed the title replace _DATE_FMT with D_FMT replace _DATE_FMT with D_T_FMT Aug 14, 2014
@salazaj
Copy link
Contributor Author

salazaj commented Aug 14, 2014

output of print_timestamp(1) and print_timestamp(2) from lib/libspl/timestamp.c

D_T_FMT 1408023168
DATE_FMT 1408023168
D_T_FMT Thu Aug 14 09:32:48 2014
DATE_FMT Thu Aug 14 09:32:48 EDT 2014

assigning fmt the format specifiers directly can yield the timezone, although at the end

fmt = "%c %Z";:  1408028529
_DATE_FMT: 1408028529
fmt = "%c %Z";:  Thu Aug 14 11:02:09 2014 EDT
_DATE_FMT: Thu Aug 14 11:02:09 EDT 2014

@salazaj salazaj changed the title replace _DATE_FMT with D_T_FMT replace _DATE_FMT with format specifiers Aug 14, 2014
Bring into conformance with ISO specification for strftime.
Set fmt="%c %Z" to yield the same string as _DATE_FMT but with
the timezone on the end.
@behlendorf
Copy link
Contributor

If the Illumos code is to be believed then _DATE_FMT was added for XPG4 (XSH4) compliance. And then fairly recently GNU picked it up as an extension, https://sourceware.org/ml/newlib/2010/msg00152.html. Presumably, this causes build failures in a different environment?

@salazaj
Copy link
Contributor Author

salazaj commented Aug 14, 2014

Yes, I'm trying to address #2567. I don't know about the other libc's but I would suspect similar behavior. I have to say, the link you gave is a bit over my head. Does this mean that bypassing nl_langinfo is not an option?

@salazaj
Copy link
Contributor Author

salazaj commented Aug 14, 2014

Ok, so if I understand this right. nl_langinfo is needed vice passing the format specifiers directly to allow for localization before going to strftime? Would a call to setlocale suffice instead?

@salazaj
Copy link
Contributor Author

salazaj commented Aug 14, 2014

Or would it be preferable to add something like this:

#ifndef _XOPEN_XPG4
#define _DATE_FMT D_T_FMT
#endif /* _XOPEN_XPG4 */

@salazaj
Copy link
Contributor Author

salazaj commented Aug 14, 2014

I'd rather not clutter up the code base with this sort of thing. So I'll close the pull request and leave this alone unless you think this needs to be addressed.

@behlendorf
Copy link
Contributor

@salazaj It might be a problem for other libc's depending on what they support. But until we find such a version of libc I'd suggest sticking with the existing implementation.

@salazaj salazaj closed this Aug 14, 2014
@salazaj salazaj mentioned this pull request Aug 17, 2014
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.

2 participants