[PATCH] D93021: [llvm-readelf/obj] - Minor cleanup of warning reporting.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D93021#2447909 <https://reviews.llvm.org/D93021#2447909>, @grimar wrote:

> In D93021#2446766 <https://reviews.llvm.org/D93021#2446766>, @MaskRay wrote:
>
>> Is this desired? For
>>
>>   reportUniqueWarning(
>>             "SHT_DYNSYM section header and DT_SYMTAB disagree about "
>>             "the location of the dynamic symbol table");
>>
>> it is fine. But for the others, a bunch of `toString` are added.
>
> It cleans up the API, leaving the single `reportUniqueWarning(const Twine &Msg)` method.
> This method has different enough signature from regular global `reportWarning`.
> It should help to encourage people to use `toString` and provide more context for error messages.
>
> Generally I think that lines like
> `this->reportUniqueWarning(toString(SymsOrErr.takeError()));` are not ideal.
> Because most of the time messages could be better if they were like:
> ``this->reportUniqueWarning("some context here: " + toString(SymsOrErr.takeError()));``
>
> I think a context could be added for many if not all of of places touched.

It seems to me like having both APIs is useful - there are occasions when there is no need to add additional context, and other occasions where there is. I think we should use whichever approach is suitable for the given situation. For example, the  version definition one in `printVersionDependencySection` is calling a method with the same parameters as the calling code - there is no additional context that the calling code can usefully provide that the callee which returned the error couldn't, so it just has to call `toString`.


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

https://reviews.llvm.org/D93021