On Fri, Apr 29, 2016 at 5:24 PM, Bill Williams <b...@cs.wisc.edu> wrote: > Taking things in patch order: > > codegen-x86.C: fine to remove rather than commenting dead code there > dyninstAPI/src/linux.C: by symmetry, use pclose if reading the first line > fails. > InstructionDecoder-aarch64.C: we'd better be null-checking and failing the > entire decode if we hit the failure cases in this patch. > BoundFactData.C: would be better to clean that interface further, but patch > is ok as far as it goes > codegen-aarch64.C: this obviously need some real cleanup, and I think we > want to work in words, but with named buffers here. blr_buf is wrong > compared to what we use as an instruction word below for the blr; > byte-swapping is obviously a mess compared to dealing with words. We'll work > on it. > proccontrol/src/mmapalloc.C: addr_size is unnecessary on ARM. In general, > if (void)ing a variable fixes warnings, the variable either should die or be > used appropriately. > symtabAPI/src/Function.C: we should be able to collapse the FunctionBase > constructors into a single ctor(void) if they're truly identical--Functions > and InlinedFunctions don't need to pass their symbol/module down to the > parent if it won't use it. > > And a general note for developers: we should prefer using scoped_ptr and a > custom destructor for file descriptors and other such things where we're > going to error-check, clean up, and bail multiple times in a function. Code > then becomes: > > scoped_ptr<FILE*> f(fopen(whatever), &fclose); > > if(!fseek(stuff)) return false; > if(fwrite(stuff) < 0) return false; > > Somewhat less error-prone, closes the same way no matter how you exit the > function. It's not an idiom we use at all yet, but seeing this list of fixes > reminded me that we really should. > > --bw
Alright, I'll rework this and submit a v2 at some point. Thanks, Peter _______________________________________________ Dyninst-api mailing list Dyninst-api@cs.wisc.edu https://lists.cs.wisc.edu/mailman/listinfo/dyninst-api
