digitalmars.D.bugs - [Issue 5059] New: String assignment in foreach loop breaks immutability
- d-bugmail puremagic.com (44/44) Oct 15 2010 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (14/14) Apr 28 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (10/10) May 24 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (12/12) May 24 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (22/22) May 24 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (13/13) May 24 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (9/9) May 24 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (8/8) May 24 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (8/8) May 24 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (7/7) May 24 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
- d-bugmail puremagic.com (11/11) Jul 11 2011 http://d.puremagic.com/issues/show_bug.cgi?id=5059
http://d.puremagic.com/issues/show_bug.cgi?id=5059 Summary: String assignment in foreach loop breaks immutability Product: D Version: D2 Platform: Other OS/Version: Windows Status: NEW Severity: normal Priority: P2 Component: DMD AssignedTo: nobody puremagic.com ReportedBy: andrej.mitrovich gmail.com 12:04:00 PDT --- Since this example uses the Registry, you need Windows to compile it: module mymodule; import std.stdio : writeln, write; import std.windows.registry; void main() { Key HKLM = Registry.localMachine; Key SFW = HKLM.getKey("software"); string[] names; foreach (Key key; SFW.keys()) { string name = key.name(); // string name = key.name().idup; // workaround for the issue names ~= name; } writeln("results:"); foreach (name; names) { write(name, ", "); } } The resulting string array has strings that are overwritten with each other, and in my case the results are similar to this: Sun Microsystems, Sun Micros, Sun , Sun Micr, Sun, Sun Mic,... And it goes like that for a hundred or so values, then switches to the next registry key name and writes more overwwritten strings like that. The commented out code is the workaround, however string to string assignment should be safe without a need for .idup. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Oct 15 2010
http://d.puremagic.com/issues/show_bug.cgi?id=5059 Jesse Phillips <Jesse.K.Phillips+D gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |Jesse.K.Phillips+D gmail.co | |m 10:13:09 PDT --- This is a bug in KeySequence's opApply. The function is reusing a char[] for every iteration, it then casts this to a string, but ends up being modified for each call to Reg_EnumKeyName_. https://github.com/D-Programming-Language/phobos/blob/master/std/windows/registry.d#L1870 -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Apr 28 2011
http://d.puremagic.com/issues/show_bug.cgi?id=5059 14:36:34 PDT --- Should I just change that line to: Key key = m_key.getKey(sName[0 .. cchName].idup); I think this makes the most sense if we're to trust that strings are immutable. I can make a pull request if this fix would be ok (my tests show that this fixes the issue). -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 24 2011
http://d.puremagic.com/issues/show_bug.cgi?id=5059 14:44:26 PDT --- I think the better solution is to change it to const(char)[]. This better reflects that the data may be changing between iterations. If you are not storing the strings from this, then it is extremely wasteful to allocate a new memory block for each iteration. You can always idup or dup if you want to. Note, I think the documentation needs to specifically state it reuses the buffer, so you should .dup or .idup if you wish to keep the data beyond the loop iteration. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 24 2011
http://d.puremagic.com/issues/show_bug.cgi?id=5059 15:12:35 PDT --- Change what exactly to const(char[])? You probably mean my "name" variable in the example code. I think this is again that issue of implicitly reusing a variable in a foreach loop. I really wish this was more explicit instead of relying on documentation, e.g. I'd love it if it were like this: foreach (ref buffer; iterator()) { } // it's reusing buffer on each iteration foreach (buffer; iterator()) { } // allocates new buffer on each iteration Maybe it's easy to reason about the current foreach mechanism when you only have one foreach loop. But when you have multiple foreach statements intertwined with dozens of different lines it's easy to miss that you should have duped something. It took me a good while before I realized what was wrong with my code until I ran it down to a simple test case like in the original post. I simply thought "hey, I'm assigning an immutable string to an immutable string, what could go wrong?". But this is more worthy of a post in the NG then to discuss it in here I guess.. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 24 2011
http://d.puremagic.com/issues/show_bug.cgi?id=5059 15:28:28 PDT --- I completely misunderstood the problem here, I thought the string was being directly returned in the opApply delegate, not a Key object. Yes, I agree, the string should be idup'd. Doing anything differently would require a redesign (which I think actually is in order, to allocate that much data to do a simple loop is incredibly wasteful), but since nobody is owning this, your solution is as good as we can get for now. Sorry for the noise. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 24 2011
http://d.puremagic.com/issues/show_bug.cgi?id=5059 16:53:41 PDT --- Yeah from a quick glance at the registry module it does seem a little fishy overall. __gshared variables, ASCII-only WinAPI calls (what if the registry key is in UTF, if that's possible?), casts to strings.. maybe this module needs to be refactored. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 24 2011
http://d.puremagic.com/issues/show_bug.cgi?id=5059 16:57:25 PDT --- Actually there's 4 opApply implementations in that module and each of them is casting char[] to a string. And other methods like getKeyName, getValueName, etc.. seem to do the same thing. Ugly! -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 24 2011
http://d.puremagic.com/issues/show_bug.cgi?id=5059 17:08:24 PDT --- I think this was a D1 module "casted" to D2 to get it to compile :) Probably would be better served reimplemented, but obviously someone would have to spend some quality time doing that. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 24 2011
http://d.puremagic.com/issues/show_bug.cgi?id=5059 17:11:28 PDT --- Btw, I think I know what you were referring to now. The opApply at line 1984 uses a delegate that takes a ref string. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 24 2011
http://d.puremagic.com/issues/show_bug.cgi?id=5059 Andrej Mitrovic <andrej.mitrovich gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution| |FIXED 08:27:55 PDT --- Fixed in 2.054 -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Jul 11 2011