[PATCH] D65515: [llvm-readobj/libObject] - Introduce a custom warning handler for `ELFFile<ELFT>` methods.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:398
+  std::function<Error(const Twine &Msg)> WarningHandler;
+  std::set<std::string> Warnings;
+
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > Should Warnings be private? Also set -> unordered_set.
> > I made it private. Making `set` to be `unordered_set` was a good idea,
> > but I can't do that now, when I have a set of pairs.
> > (It just doesn't compile, the problem is that (taken from stackoverflow): 
> > "std::unordered_set is using std::hash template to compute hashes for its entries and there is no std::hash specialization for pairs.")
> > So I had to use `set`, it does not seem to be critical here.
> You could always implement std::hash for pairs, but I agree that it's not important.
I found there was no reason to store the file name here, because a new dumper object actually is
created for each input object. So I made it to be `unordered_set` just like you suggested initially.


================
Comment at: tools/llvm-readobj/llvm-readobj.h:42
+
+  // TODO: This one is depricated. Use one with a Input name below.
   template <class T> T unwrapOrError(Expected<T> EO) {
----------------
grimar wrote:
> jhenderson wrote:
> > depricated -> deprecated
> > 
> > Do you plan to remove this function soon? I don't see why you shouldn't (apart from time to write it...!)
> > 
> > What about the ErrorOr version? Should that be updated too?
> > Do you plan to remove this function soon? I don't see why you shouldn't (apart from time to write it...!)
> I'd be happy, so yes, I'd do that.
> 
> > What about the ErrorOr version? Should that be updated too?
> May be. (I just did not look deep yet about which functions should be eliminated and which cant't be).
> I'll probably add a comment.
>> What about the ErrorOr version? Should that be updated too?
>
>May be. (I just did not look deep yet about which functions should be eliminated and which cant't be).
>I'll probably add a comment.

So I think it should be updated, but not in this patch. I can't just add a comment about deprication,
because we do not have a version with `StringRef Input` yet. (And it is not needed for this patch).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65515/new/

https://reviews.llvm.org/D65515