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

[Bill Williams <bill@xxxxxxxxxxx>]

On 04/28/2016 06:28 PM, Peter Foley wrote:
> On Thu, Apr 28, 2016 at 5:02 PM, Bill Williams <bill@xxxxxxxxxxx> wrote:
> > I'll give this one a more detailed look tomorrow. Unused params in temporary stubs are fine to silence via anonymizing parameters; unused error codes need good handling. And I'm mostly okay with comparisons that can't fail when they're making the facts being asserted more legible.
> >
> > I really need to check the dead code in context and make sure we're not missing functionality in a stupid way, too.
> Sure, I marked this one as RFC for a reason.
> I *think* I figured out everything, but it's entirely possible that I
> missed something.
> I'd be more than happy to split this up if needed.
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