llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration
authorDavid Blaikie <dblaikie@gmail.com>
Tue, 11 Dec 2018 00:09:06 +0000 (00:09 +0000)
committerDavid Blaikie <dblaikie@gmail.com>
Tue, 11 Dec 2018 00:09:06 +0000 (00:09 +0000)
commitba005aa43cfdb5b52e06245557de970c33a50a59
treecbe42b72a0cae1a83a6c45022e35e20c3dc90820
parent99bc2b21328c79a2911603db66d955c08bbd8379
llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration

Using an Error as an out parameter from an indirect operation like
iteration as described in the documentation (
http://llvm.org/docs/ProgrammersManual.html#building-fallible-iterators-and-iterator-ranges
) seems to be a little fussy - so here's /one/ possible solution, though
I'm not sure it's the right one.

Alternatively such APIs may be better off being switched to a standard
algorithm style, where they take a lambda to do the iteration work that
is then called back into (eg: "Error e = obj.for_each_note([](const
Note& N) { ... });"). This would be safer than having an unwritten
assumption that the user of such an iteration cannot return early from
the inside of the function - and must always exit through the gift
shop... I mean error checking. (even though it's guaranteed that if
you're mid-way through processing an iteration, it's not in an  error
state).

Alternatively we'd need some other (the super untrustworthy/thing we've
generally tried to avoid) error handling primitive that actually clears
the error state entirely so it's safe to ignore.

Fleshed this solution out a bit further during review - it now relies on
op==/op!= comparison as the equivalent to "if (Err)" testing the Error.
So just like an Error must be checked (even if it's in a success state),
the Error hiding in the iterator must be checked after each increment
(including by comparison with another iterator - perhaps this could be
constrained to only checking if the iterator is compared to the end
iterator? Not sure it's too important).

So now even just creating the iterator and not incrementing it at all
should still assert because the Error has not been checked.

Reviewers: lhames, jakehehrlich

Differential Revision: https://reviews.llvm.org/D55235

llvm-svn: 348811
llvm/include/llvm/Object/ELFTypes.h
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp