Re: [DynInst_API:] [RFC PATCH] Cleanup warnings

[Peter Foley <pefoley2@xxxxxxxxxxx>]

On Fri, Apr 29, 2016 at 5:24 PM, Bill Williams <bill@xxxxxxxxxxx> 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