[PATCH] D87987: [llvm-objcopy][NFC] refactor error handling. part 3.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

> I was actually referring more to the style that this patch series initially looked at, which is based on something similar to what I did in the DebugLine parser, which in turn inspired my lightning talk on error handling in libraries three(?) years ago. In other words, the client would provide one or more callbacks for different degrees of severity of problems, which llvm-objcopy's code could then call when an error occurred.



> I'm not convinced there are many points in the llvm-objcopy code where continuing after failure makes much sense, personally, so just storing an error state for reporting later seems unnecessary?

Right, that patch initially was done in that manner - the client provide one or more callbacks for different degrees of severity of problems. The argument against such a solution was that there are only fatal_errors currently, which could be reported using Expected<>/Error returning values. It looks to me that It is OK to use that(Expected<>/Error)  solution until we need to report other errors(warning/recoverable error). i.e. Let`s create handlers when we would need to report other kinds of errors.



================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:321-322
     Object &Obj, SectionPred &RemovePred,
     function_ref<bool(const SectionBase &)> shouldReplace,
-    function_ref<SectionBase *(const SectionBase *)> addSection) {
+    function_ref<Expected<SectionBase *>(const SectionBase *)> AddSection) {
   // Build a list of the debug sections we are going to replace.
----------------
jhenderson wrote:
> I would either replace both these names or neither of them, since these are the variable names already. I think we want to avoid an inconsistent style between `shouldReplace` and `AddSection`.
My own preference is that it is OK to fix small style issues if they were encountered during making some patch. But it is often rejected with the reason - "It should be separate patch". Thus I am trying to change only lines affected by patch. In this concrete case clang-tidy reported style issue - So I fixed it. If it is OK - I would change  shouldReplace also.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87987