On 12/16/2013 11:20 AM, Matthew LeGendre wrote: > I took a walk through the ProcControlAPI/StackwalkerAPI fixes and didn't > see any issues.
Thanks. > On Mon, 16 Dec 2013, Bill Williams wrote: >> * Symtab's Object copy constructor: pretty sure it should copy all the >> fields >> if it's going to exist. The secondary point, of course, is that all >> Symtabs/Objects/AObjects should be reference counted and (in principle) >> should never be copied, so the more accurate fix is to replace them with >> private versions etc. I was generally trying to stay away from semantic changes, addressing only the problem at hand. So if a copy constructor missed some fields, I assumed for the moment that those fields shouldn't be copied. Sometimes that's intentional, but still everything should get *some* initialization. We can easily copy more though if you want. Since the Objects and AObjects are not in public headers, I could just try making those into private unimplemented copy constructors, and see if anything breaks. I'm more wary of class Symtab though, since its constructors are public and exported - that's a design call for you. >> * The linux_kludges assorted fixes look much simpler and cleaner, which is >> great, but I also have to ask if these are still needed/used anywhere (or if >> it's safe to replace them with POSIX equivalents in some cases). As a >> general >> rule, things in common that are not actually domain-specific are things that >> we'd like to get rid of but (at the time we wrote them) there wasn't a >> sufficiently cross-platform alternative. (See also: pdvector. The cruft goes >> a long way back here.) These rewritten functions are a bit of an exception as this patch series goes, but I still wasn't looking much further than the functions themselves. Maybe it deserves a wider audit in the near future. In general, I agree, some things could use more from POSIX, or failing that, from boost. After I finish my compiler-warnings and static-analyzer crusade, maybe I can try my hand at some of these more general janitorial tasks. Got a list? >> * Not 100% positive that all of the fallthroughs that you left as >> fallthrough >> are non-bugs, but in the worst case they're not making behavior any worse. >> I'll review those in more detail this afternoon and see if I find anything >> that needs fixing. Sure, let me know. This is why I separated my commits for intentional fallthrough vs added breaks, so this would stand out. Thanks, Josh _______________________________________________ Dyninst-api mailing list Dyninst-api@cs.wisc.edu https://lists.cs.wisc.edu/mailman/listinfo/dyninst-api
