Hi, anonym wrote (26 Jul 2013 12:46:34 GMT) : > branch: test/reorg
The general idea looks totally sensible to me, and I'm happy to read it will bring some performance improvements with it. Now, let's inspect some of the proposed code in detail, as I have some concerns. To start with, I'm sorry if any comment that follows results from my poor knowledge of Ruby. Also, if some of the issues mentionned are not regressions, please make it clear so that this branch can be merged first, and the old problems dealt with later, one step at a time. Here we go. In the VMNet class, the clean_up and destroy methods do very similar things. It's not obvious to me when I should use the one as opposed to the other. May you please add comments or rdoc or something to explain how their purpose is different? Also, it may be the Ruby way to design OO stuff, but having 2 methods (clean_up and destroy) implicitly depend on a 3rd one having been successfully run on the instance before looks fragile to me. Perhaps instead use a read-only accessor (possibly already provided by attr_reader?) that errors out if the attribute wasn't set yet? Or simply force the `update' step, that apparently is cricitally needed for the instance to do anything useful, to run at construction time? And while we're at it, once `clean_up' depends on `update' to work properly, I don't get why we don't use the @net attribute that was initialized in `update', and instead look it up again by name. Or perhaps that `net' there isn't the same as the @net instance attribute? This: > net_xml = REXML::Document.new(@vmnet.net.xml_desc) > @ip = net_xml.elements['network/ip/dhcp/host/'].attributes['ip'] > @mac = net_xml.elements['network/ip/dhcp/host/'].attributes['mac'] in the VM class, looks like broken encapsulation to me: the VM class shouldn't need to go that far into low-level implementation details of the @vmnet instance. Perhaps provide helper `ip' and `mac' public methods in VMNet (with some kind of caching or memoization to avoid recomputing it every time)? I look forward to see this improved and merged! :) Given that this was not a trivial merge, unless you intend to address all these problems in the next few days, please create a ticket so that these commits from April do not slip through for months again. _______________________________________________ tails-dev mailing list [email protected] https://mailman.boum.org/listinfo/tails-dev
