Issue 10956043: code review 10956043: time: find correct zone abbreviations even on non-Engli... (Closed)
Descriptiontime: find correct zone abbreviations even on non-English windows systems
Fixes issue 5783
Patch Set 1 #Patch Set 2 : diff -r 0d2b8690d896 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 000ecca1178d https://go.googlecode.com/hg/ #
Total comments: 14
Patch Set 4 : diff -r d2b753bcb7df https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 5 : diff -r d0c957701447 https://go.googlecode.com/hg/ #MessagesTotal messages: 9
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
Windows is a wonder. https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... File src/pkg/time/zoneinfo_windows.go (right): https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:21: // getKeyValue retrieves string value kname associated with an open registry key kh. s/string/the string/ s/an/the/ https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:23: var buf [200]uint16 where does 200 come from? (seems kinda big in any case) https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:25: n := uint32(len(buf) * 2) // api expects array of bytes, not uint16 s/api/RegQueryValueEx's signature/ https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:36: // matchZoneKey checks if stdname and dstname matches correspondent "Std" // matchZoneKey checks if stdname and dstname match the corresponding "Std" // and "Dlt" key values in the kname key stored under the open registry key zones. are they really spelled "Std" and "Dlt" but: i don't understand how a key value in a key can be stored under a key. there are too many keys involved. the registry is stranger than i thought or the comment could be clarified. maybe both. https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:63: // toEnglishName searches registry for English name of timezone with // toEnglishName searches the registry for an English name of a time zone whose // zone names are stdname and dstname and returns the English name. https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:90: return "", errors.New(`English name for "` + stdname + `" is not found`) maybe return "", errors.New(`English name for time zone "` + stdname + `" not found in registry`) https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:110: // prehaps stdName is non-English, try to convert it // Perhaps stdName is not English. Try to convert it.
Sign in to reply to this message.
I realize I just wrote "Windows is" without flinching.
Sign in to reply to this message.
> Windows is a wonder. No argument. Especially this time zones part is bad. But it grows on you :-) Nothing is perfect. Alex https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... File src/pkg/time/zoneinfo_windows.go (right): https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:21: // getKeyValue retrieves string value kname associated with an open registry key kh. On 2013/07/09 03:27:32, r wrote: > s/string/the string/ > s/an/the/ Done. https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:23: var buf [200]uint16 On 2013/07/09 03:27:32, r wrote: > where does 200 come from? This needs to be large enough to fit zone descriptions. I have collected all posible candidates in zoneinfo_abbrs_windows.go. But, as I discovered later, these could at least be in non-English too. > (seems kinda big in any case) I agree. Changed to 50. I have also changed similar array in toEnglishName to 50 too. https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:25: n := uint32(len(buf) * 2) // api expects array of bytes, not uint16 On 2013/07/09 03:27:32, r wrote: > s/api/RegQueryValueEx's signature/ Done. https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:36: // matchZoneKey checks if stdname and dstname matches correspondent "Std" On 2013/07/09 03:27:32, r wrote: > // matchZoneKey checks if stdname and dstname match the corresponding "Std" > // and "Dlt" key values in the kname key stored under the open registry key > zones. Done. > > are they really spelled "Std" and "Dlt" Yes. This is an example of a key with its values: [HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones\Alaskan Standard Time] "MUI_Display"="@tzres.dll,-220" "MUI_Dlt"="@tzres.dll,-221" "MUI_Std"="@tzres.dll,-222" "Display"="(UTC-09:00) Alaska" "Dlt"="Alaska (czas letni)" "Std"="Alaska (czas standardowy)" "TZI"=hex:1c,02,00,00,00,00,00,00,c4,ff,ff,ff,00,00,0b,00,00,00,01,00,02,00,00,\ 00,00,00,00,00,00,00,03,00,00,00,02,00,02,00,00,00,00,00,00,00 > > but: i don't understand how a key value in a key can be stored under a key. > there are too many keys involved. the registry is stranger than i thought or the > comment could be clarified. maybe both. I am sure it is me. Registry keys are similar to directories - they can contain other keys (like directories) and values (like files). You need to "open" keys before you can interrogate them - enumerate keys inside or read / write values. All "open" operations require an already "opened" key - this key will be used as a root in the open operation. For example, to have access to [HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones\Alaskan Standard Time], you would provide handle to (already opened) HKEY_LOCAL_MACHINE and a key name "SOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones\Alaskan Standard Time", alternatively you could provide a handle to (already opened) "HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones" key and a key name "Alaskan Standard Time". Some Windows root key opened handles are just fixed numbers (just like stdin and stdout), for example HKEY_LOCAL_MACHINE and HKEY_CURRENT_USER an some others. My matchZoneKey function here takes handle to already opened key "HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones" and a key name (for example "Alaskan Standard Time") and checks to see if the key ("HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones\Alaskan Standard Time") has values named "Std" and Dlt" and these values match provided stdname and dstname parameters. If you have better way to document that, I am happy to use it. This is a second place in Go library where we use registry access, the other one is mime. Do you think it is worth creating some common "handle windows registry" api in syscall to simplify code here? https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:63: // toEnglishName searches registry for English name of timezone with On 2013/07/09 03:27:32, r wrote: > // toEnglishName searches the registry for an English name of a time zone whose > // zone names are stdname and dstname and returns the English name. Done. https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:90: return "", errors.New(`English name for "` + stdname + `" is not found`) On 2013/07/09 03:27:32, r wrote: > maybe > > return "", errors.New(`English name for time zone "` + stdname + `" not found in > registry`) Done. https://codereview.appspot.com/10956043/diff/5001/src/pkg/time/zoneinfo_windo... src/pkg/time/zoneinfo_windows.go:110: // prehaps stdName is non-English, try to convert it On 2013/07/09 03:27:32, r wrote: > // Perhaps stdName is not English. Try to convert it. Done.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, r@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
An interface to the registry would make sense, perhaps in syscall or perhaps outside. But that can be a separate discussion, not part of this CL.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/10956043/diff/13001/src/pkg/time/zoneinfo_wind... File src/pkg/time/zoneinfo_windows.go (right): https://codereview.appspot.com/10956043/diff/13001/src/pkg/time/zoneinfo_wind... src/pkg/time/zoneinfo_windows.go:23: var buf [50]uint16 add a comment about 50. a precis of the text you had in the CL response would be good.
Sign in to reply to this message.
On 2013/07/10 02:58:48, r wrote: > An interface to the registry would make sense, perhaps in syscall or > perhaps outside. But that can be a separate discussion, not part of > this CL. Sure. I'll think about it. The hard part is to make it future proof. We don't use registry extensively enough to cover all possible scenarios. Alex
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=2fc4f0ee4234 *** time: find correct zone abbreviations even on non-English windows systems Fixes issue 5783 R=golang-dev, r CC=golang-dev https://codereview.appspot.com/10956043 https://codereview.appspot.com/10956043/diff/13001/src/pkg/time/zoneinfo_wind... File src/pkg/time/zoneinfo_windows.go (right): https://codereview.appspot.com/10956043/diff/13001/src/pkg/time/zoneinfo_wind... src/pkg/time/zoneinfo_windows.go:23: var buf [50]uint16 On 2013/07/10 03:01:54, r wrote: > add a comment about 50. a precis of the text you had in the CL response would be > good. Done.
Sign in to reply to this message.
|
